Re: [PATCH] SBuf c-string comparisons

Patch attached for just these SBuf and related unit test changes.

On 11/04/2014 10:24 a.m., Alex Rousskov wrote:
> On 04/10/2014 05:47 AM, Amos Jeffries wrote:
<snip>
>
>> + 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).

Done and const n.

>
>> + /// 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.
>

The situation for C-string is different from SBuf. Since SBuf contain
lengths and guarantee non-NULL buffers we are not using the N parameter
for any bounds safety.
 Whereas with C-string it is required for over-read bounds safety.

Done anyway.

>
>> + 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.
>

Added with a slight modification to use the existing sign() converter.
Thank you, that let me find and fix several off-by-1 bugs and the
terminator handling bugs relatively easily.

>
>> +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().
>

Combined the two now same as the SBuf variant. With the above and some
added checks for handling NULL pointers and empty strings.

>
>> + 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.
>

Done all including n.

Amos