Re: [PATCH] SBuf conversion of HttpRequestMethod

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.

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.

* 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);

New stuff:

> + int cmp(const char *s, size_t n) const;
> + int caseCmp(const char *s, size_t n) const;

SBuf is currently using size_type, not size_t for comparison methods.
Size_t is not used anywhere in SBuf.h AFAICT. Do we have to add size_t
into the mix? If you start using size_type instead, please note that the
type casting inside the methods will go away (but new type casts in the
callers might be needed).

> + /// comparison with a C-string
> + int cmp(const char *s, size_t n) const;
> +
> + /// case-insensitive comparison with a C-string
> + int caseCmp(const char *s, size_t n) const;

There is no good reason to force callers to call length(s) IMO. Please
add a default npos value for n. This will also make the new methods more
consistent with the existing ones.

> + if (rv != 0)
> + return rv;
> + if (length() == n)
> + return 0;
> + if (length() > n)
> + return 1;
> + return -1;

This seemingly correct logic actually leads to wrong comparison results:

    strncmp("foo", "f", 1) is 0
but
    SBuf("foo").cmp("f", 1) is +1

If you adapted that code from the existing SBuf::compare(), please note
that the similar compare() code works because it is preceded by
truncation of _both_ strings to [up to] n characters.

To minimize code duplication, you probably want to do here what old
compare() does for old cmp() and caseCmp().

Finally, this bug also exposes the lack of comparison test cases. The
attached unpolished patch adds a few. If you like them, please integrate
with your changes and polish as needed. See TODO in patch preamble for a
better approach though.

> +int
> +SBuf::cmp(const char *s, size_t n) const
> +{
> + assert(s != NULL);

It may be better to allow NULL c-strings with zero n:

    if (!n)
        return 0;
    assert(s);

I do not insist on this change.

Same for the other caseCmp().

> + size_type byteCompareLen = min(n, static_cast<size_t>(length()));
...
> + int rv = memcmp(buf(), s, byteCompareLen);
...
> + size_type byteCompareLen = min(n, static_cast<size_t>(length()));
...
> + int rv = memcasecmp(buf(), s, byteCompareLen);

Make these variables const if possible.

I would also make the "n" parameter itself const but that should be done
for other cmp() methods as well then.

HTH,

Alex.