<div dir="ltr">+1 then<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 3, 2015 at 7:20 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"><span class="">On 4/07/2015 4:09 a.m., Kinkie wrote:<br>
> Hi,<br>
> 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>
<br>
</span>In this case (./src/adaptation/icap/Options.cc) it makes the return type<br>
non-const.<br>
<span class=""><br>
<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>
<br>
</span>Done.<br>
<span class=""><br>
<br>
> <kinkie> that's one fewer copy<br>
> <kinkie> I wouldn't so systematically mention c_str as a performance<br>
> 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<br>
> not regressions (ie replace a copy with a copy)<br>
<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>
<br>
</span>Done.<br>
<span class=""><br>
> <kinkie> src/gopher.cc -> xstrncpy(request, path.c_str(), MAX_URL);<br>
> <kinkie> maybe<br>
> xstrncpy(request,path.rawContent(),min(path.length(),MAX_URL)) could do<br>
> <kinkie> IIRC xstrncpy null-terminates, right?<br>
<br>
</span>It does. Using:<br>
  SBuf path = tok.remaining().substr(0, MAX_URL);<br>
  xstrncpy(request, path.rawContent(), path.length());<br>
<span class=""><br>
<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<br>
> improvement because you'd be keepign info about length around<br>
<br>
</span>Done.<br>
<span class=""><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<br>
> variant<br>
<br>
</span>Done.<br>
<span class=""><br>
> <kinkie> same in internalCheck<br>
> <kinkie> etc<br>
> <yadi> hmm<br>
<br>
</span>Done. Using startsWith there.<br>
<span class=""><br>
<br>
> <kinkie> UrnState::getHost; why is the input parameter a SBuf& (not const)?<br>
> <kinkie> in the implementation of UrnState::getHost there's inconsistent<br>
> spacing in result=xstrndup(urlpath.rawContent(),p-1);<br>
> <kinkie> in UrnState::RequestNeedsMenu() the length() check is redundant;<br>
> 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>
<br>
</span>Done. And removing RequestNeedsMenu entirely now that its one use is so<br>
simplified.<br>
<span class=""><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>
</span>The path() getter return is const so not modifiable. Cycling via<br>
path(...) setter also ensures touch() is called, preparation for<br>
canonical() being made a method.<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
Amos<br>
<br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature">    Francesco</div>
</div>