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

[Kinkie]

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 ?
<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())
<yadi> nod
<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
<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?
<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
<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
<kinkie> same in internalCheck
<kinkie> etc
<yadi> hmm
<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 ;)
<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


I see no obvious issues with the code and you mention you have run-tested
it, so +1 with the fixes above.

On Sun, Jun 28, 2015 at 1:21 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> This takes another step towards bug 1961 closure by shuffling the
> HttpRequest::urlpath member into class URL.
>
> These changes appear to work nicely. But I have not been able to test
> all code paths that have logic change so a second pair of eyes on it
> would be appreciated.
>
> Amos
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>
>


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