On 03/03/2013 10:45 PM, Amos Jeffries wrote:
> http://master.squid-cache.org/~amosjeffries/patches/SBuf_fixes_mk3.patch
* SBuf::rfind(const SBuf &needle, SBuf::size_type endPos) const
> + // on empty hay std::string returns npos
> + if (length() < needle.length())
> + return SBuf::npos;
The comment is inaccurate: empty std::string returns npos only when the
needle is not empty. Besides, the if-statement does not check for an
empty hay at all. The above code is correct -- if needle does not fit we
can only return npose, of course, but I do not think we need a reference
to std::string in this case.
> // on empty needle std::string returns the position the search starts
> if (needle.length() == 0)
> return endPos;
The comment is a little misleading: std::string actually returns its
length when searching start exceeds that length:
> std::string("a2").rfind("", 0) = 0
> std::string("a2").rfind("", 1) = 1
> std::string("a2").rfind("", 2) = 2
> std::string("a2").rfind("", 3) = 2
The code itself is correct because you adjust endPos before getting to
that if-statement. Instead of trying hard to describe what std::string
does correctly, I suggest just saying something like this:
// match std::string::rfind() behavior on empty needles
* SBuf::rfind(char c, SBuf::size_type endPos) const
> + // on empty hay std::string returns size of hay
> + if (length() < 1)
> return SBuf::npos;
The comment is out of sync with the correct code. Here we do not need to
refer to std::string IMO.
> There is a rather strange problem with using memrchr(). If you pass it
> the accurate count of bytes to check it fails to match start/end of hay
> properly in sub-string cases.
It does not in my tests:
> memrchr("a2", '2', 0) = 0
> memrchr("a2", '2', 1) = 0
> memrchr("a2", '2', 2) = 0x8048ba1
> memrchr("a2", '2', 3) = 0x8048ba1
> memrchr("2a", '2', 0) = 0
> memrchr("2a", '2', 1) = 0x8048c61
> memrchr("2a", '2', 2) = 0x8048c61
> memrchr("2a", '2', 3) = 0x8048c61
> memrchr("a2a", '2', 0) = 0
> memrchr("a2a", '2', 1) = 0
> memrchr("a2a", '2', 2) = 0x8048c02
> memrchr("a2a", '2', 3) = 0x8048c02
All of the above seem to work as expected when the "accurate count of
bytes to check" is passed.
> I have had to use endPos+1 and artificially manipulate the situations
> where endPos == length().
Your mk3 code seems to be correct, with no strange problems in this
area: You are correctly converting character position to the number of
bytes to scan. I recommend replacing the "weirdness" comment with
something like "convert character position to the number of characters
to scan".
HTH,
Alex.
