<div dir="ltr"><div><div>Hi,<br></div>my review (copy-paste from IRC)<br><br><kinkie> SBuf::size_type urlLen = ...<br><kinkie> what about using auto?<br><yadi> auto seems to only work sometimes.<br><kinkie> O_O ?<br><kinkie> Looks strange in such a trivial case<br><kinkie> src/client_side.cc<br><kinkie> there's a SBuf tmp(..); strcpy().<br><kinkie> Can't we do better? <br><kinkie> we know the length of the SBuf after all<br><kinkie> you could xstrncpy(..., ...->length())<br><yadi> nod<br><kinkie> that's one fewer copy<br><kinkie> I wouldn't so systematically mention c_str as a performance regression<br><kinkie> e.g. when doing mime lookup: yes it reallocs but it's a few bytes<br><kinkie> I'd rather TODO: refactor mime*<br><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)<br><kinkie> xstrndup(tmp.c_str(),tmp.length());<br><kinkie> why go c_str() if you know the length?<br><kinkie> I'd go rawContent()<br><kinkie> and later on xstrdup(), forgetting you know the length<br><kinkie> this is all in FtpGateway.cc<br><kinkie> src/gopher.cc -> xstrncpy(request, path.c_str(), MAX_URL);<br><kinkie> maybe xstrncpy(request,path.rawContent(),min(path.length(),MAX_URL)) could do<br><kinkie> IIRC xstrncpy null-terminates, right?<br><kinkie> HttpStateData::buildRequestPrefix<br><kinkie> maybe it's possible to have url the other way around<br><kinkie> SBuf url;<br><kinkie> if (_peer && !_peer->options.originserver)<br><kinkie> rul = SBuf(urlCanonical(request);<br><kinkie> forgot a )<br><kinkie> and then in the else branch just use url = url.path()<br><kinkie> and fix the mb->appendf().<br><kinkie> I'm willing to bet it'll end up being a small performance improvement because you'd be keepign info about length around<br><kinkie> src/internal.cc<br><kinkie> why use upath.cmp(storeDigestUri) == 0 ?<br><kinkie> It's easier to just upath == storeDigestUri<br><kinkie> more readable<br><kinkie> the only case where it makes sense to use cmp is the caseCmp variant<br><kinkie> same in internalCheck<br><kinkie> etc<br><yadi> hmm<br><kinkie> UrnState::getHost; why is the input parameter a SBuf& (not const)?<br><kinkie> in the implementation of UrnState::getHost there's inconsistent spacing in result=xstrndup(urlpath.rawContent(),p-1);<br><kinkie> in UrnState::RequestNeedsMenu() the length() check is redundant; caseCmp does it for you<br><kinkie> (IIRC)<br><yadi> caseCmp() returns false if SBuf are different length<br><yadi> .menu is a prefix on one Sbuf<br><kinkie> use startsWith then<br><kinkie> that's a perfect use case ;)<br><kinkie> r->url.path(r->url.path().substr(5)); // strip prefix "menu."<br><kinkie> you could use SBuf::chop there maybe<br><kinkie> yes, looks like it<br><kinkie> r->url.path.chop(5) should do the trick<br><br><br></div>I see no obvious issues with the code and you mention you have run-tested it, so +1 with the fixes above.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jun 28, 2015 at 1:21 PM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This takes another step towards bug 1961 closure by shuffling the<br>
HttpRequest::urlpath member into class URL.<br>
<br>
These changes appear to work nicely. But I have not been able to test<br>
all code paths that have logic change so a second pair of eyes on it<br>
would be appreciated.<br>
<span class="HOEnZb"><font color="#888888"><br>
Amos<br>
</font></span><br>_______________________________________________<br>
squid-dev mailing list<br>
<a href="mailto:squid-dev@lists.squid-cache.org">squid-dev@lists.squid-cache.org</a><br>
<a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a><br>
<br></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature">    Francesco</div>
</div>