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

[Amos Jeffries]

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