Re: [PATCH] SBuf conversion of HttpRequestMethod

On 11/04/2014 10:24 a.m., Alex Rousskov wrote:
> On 04/10/2014 05:47 AM, Amos Jeffries wrote:
>> New patch attached.
>> This clears up the requests from both of you.
>
> Not all of them, and there is also a new serious bug. Details below.
>

Okay. So the critical issue(s) are in the half-baked SBuf API extension.
Posting that as a separate patch and blocking this patch until that is
complete.

I believe the attached patch is complete regarding the method conversion.

>
> Old stuff:
>
> * The usually needless SBuf allocation for common methods is still there:
>
>> + SBuf str(begin, end-begin);
>> +
>> + // TODO: Optimize this linear search.
>> for (++theMethod; theMethod < Http::METHOD_ENUM_END; ++theMethod) {
> ...
>
> It should be avoided, and I thought adding SBuf::cmp(c-string) methods
> was done specifically to avoid it. I do not even see those new methods
> used in this code.
>

Not the loop. Just to avoid the need for "SBuf str(begin, end-begin);"
allocation at the top there.
 The loop still needs to take place using the c-string compare methods.

This version of the patch includes that, BUT becomes blocked by the SBuf
API changes as a result.

>
> * Some changed debug lines still quote things for no good (IMO) reason.
> For example:
>
>
>> + debugs(17, 3, clientConn << ": Fetching '" << request->method << ' ' << entry->url() << '\'');
>
>> + debugs(88, 4, '\'' << r->method << ' ' << url << '\'');
>
>> + debugs(88, 4, '\'' << http->request->method << ' ' << http->uri << '\'');
>
>
> There are probably more of these -- I have not checked the whole patch
> this time. You may, of course, insist that this 'quoting' must stay, but
> removing it was one of my suggestions. If needless quoting is removed,
> the above would look like these:
>
>> debugs(17, 3, clientConn << ": Fetching " << request->method << ' ' << entry->url());
>> debugs(88, 4, r->method << ' ' << url);
>> debugs(88, 4, http->request->method << ' ' << http->uri);
>

Oh! Log output quoting was what you meant. Thought you were on about the
"" vs '' string wrappings in the code itself.

Oh well, both are now fixed.

Amos