[squid-dev] [PATCH] Refactor path strings into class URL

[Kinkie]

+1 then

On Fri, Jul 3, 2015 at 7:20 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 4/07/2015 4:09 a.m., Kinkie wrote:
> > Hi,
> > my review (copy-paste from IRC)
> >
> > <kinkie> SBuf::size_type urlLen = ...
> > <kinkie> what about using auto?
> > <yadi> auto seems to only work sometimes.
> > <kinkie> O_O ?
>
> In this case (./src/adaptation/icap/Options.cc) it makes the return type
> non-const.
>
>
> > <kinkie> Looks strange in such a trivial case
> > <kinkie> src/client_side.cc
> > <kinkie> there's a SBuf tmp(..); strcpy().
> > <kinkie> Can't we do better?
> > <kinkie> we know the length of the SBuf after all
> > <kinkie> you could xstrncpy(..., ...->length())
>
> Done.
>
>
> > <kinkie> that's one fewer copy
> > <kinkie> I wouldn't so systematically mention c_str as a performance
> > regression
> > <kinkie> e.g. when doing mime lookup: yes it reallocs but it's a few
> bytes
> > <kinkie> I'd rather TODO: refactor mime*
> > <yadi> alex complains if I dont. Ive avoided it on the ones I can see are
> > not regressions (ie replace a copy with a copy)
>
> > <kinkie> xstrndup(tmp.c_str(),tmp.length());
> > <kinkie> why go c_str() if you know the length?
> > <kinkie> I'd go rawContent()
> > <kinkie> and later on xstrdup(), forgetting you know the length
> > <kinkie> this is all in FtpGateway.cc
>
> Done.
>
> > <kinkie> src/gopher.cc -> xstrncpy(request, path.c_str(), MAX_URL);
> > <kinkie> maybe
> > xstrncpy(request,path.rawContent(),min(path.length(),MAX_URL)) could do
> > <kinkie> IIRC xstrncpy null-terminates, right?
>
> It does. Using:
>   SBuf path = tok.remaining().substr(0, MAX_URL);
>   xstrncpy(request, path.rawContent(), path.length());
>
>
> > <kinkie> HttpStateData::buildRequestPrefix
> > <kinkie> maybe it's possible to have url the other way around
> > <kinkie> SBuf url;
> > <kinkie> if (_peer && !_peer->options.originserver)
> > <kinkie> rul = SBuf(urlCanonical(request);
> > <kinkie> forgot a )
> > <kinkie> and then in the else branch just use url = url.path()
> > <kinkie> and fix the mb->appendf().
> > <kinkie> I'm willing to bet it'll end up being a small performance
> > improvement because you'd be keepign info about length around
>
> Done.
>
> > <kinkie> src/internal.cc
> > <kinkie> why use upath.cmp(storeDigestUri) == 0 ?
> > <kinkie> It's easier to just upath == storeDigestUri
> > <kinkie> more readable
> > <kinkie> the only case where it makes sense to use cmp is the caseCmp
> > variant
>
> Done.
>
> > <kinkie> same in internalCheck
> > <kinkie> etc
> > <yadi> hmm
>
> Done. Using startsWith there.
>
>
> > <kinkie> UrnState::getHost; why is the input parameter a SBuf& (not
> const)?
> > <kinkie> in the implementation of UrnState::getHost there's inconsistent
> > spacing in result=xstrndup(urlpath.rawContent(),p-1);
> > <kinkie> in UrnState::RequestNeedsMenu() the length() check is redundant;
> > caseCmp does it for you
> > <kinkie> (IIRC)
> > <yadi> caseCmp() returns false if SBuf are different length
> > <yadi> .menu is a prefix on one Sbuf
> > <kinkie> use startsWith then
> > <kinkie> that's a perfect use case ;)
>
> Done. And removing RequestNeedsMenu entirely now that its one use is so
> simplified.
>
> > <kinkie> r->url.path(r->url.path().substr(5)); // strip prefix "menu."
> > <kinkie> you could use SBuf::chop there maybe
> > <kinkie> yes, looks like it
> > <kinkie> r->url.path.chop(5) should do the trick
> >
>
> The path() getter return is const so not modifiable. Cycling via
> path(...) setter also ensures touch() is called, preparation for
> canonical() being made a method.
>
>
> Amos
>
>


-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150704/d8c90ca1/attachment.html>