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.
Added test cases for SBuf::cmp and SBuf::caseCmp methods,
including pending c-string comparison methods.
Unpolished. For example, it is difficult to know which test case failed.
Also missing new function descriptions.
TODO: We should add comparison test cases to automated tests with generated
data (perhaps instead of these manual ones).
=== modified file 'src/tests/testSBuf.cc'
--- src/tests/testSBuf.cc 2014-02-08 13:36:42 +0000
+++ src/tests/testSBuf.cc 2014-04-10 22:31:11 +0000
@@ -227,74 +227,123 @@
// note: can't use cppunit's CPPUNIT_TEST_EXCEPTION because TextException asserts, and
// so the test can't be properly completed.
void
testSBuf::testSubscriptOpFail()
{
char c;
c=literal.at(literal.length()); //out of bounds by 1
//notreached
std::cout << c << std::endl;
}
static int sign(int v)
{
if (v < 0)
return -1;
if (v>0)
return 1;
return 0;
}
+static void
+testComparisonStdCheck(int res1, int res2) {
+ // normalize result values to -1, 0, +1
+ if (res1)
+ res1 = res1 > 0 ? +1 : -1;
+ if (res2)
+ res2 = res2 > 0 ? +1 : -1;
+ CPPUNIT_ASSERT_EQUAL(res1, res2);
+}
+
+static void
+testComparisonStdFull(const char *left, const char *right) {
+ testComparisonStdCheck(strcmp(left, right), SBuf(left).cmp(right));
+ testComparisonStdCheck(strcmp(left, right), SBuf(left).cmp(SBuf(right)));
+ testComparisonStdCheck(strcasecmp(left, right), SBuf(left).caseCmp(right));
+ testComparisonStdCheck(strcasecmp(left, right), SBuf(left).caseCmp(SBuf(right)));
+}
+
+static void
+testComparisonStdN(const char *left, const char *right, const size_t n) {
+ testComparisonStdCheck(strncmp(left, right, n), SBuf(left).cmp(right, n));
+ testComparisonStdCheck(strncmp(left, right, n), SBuf(left).cmp(SBuf(right), n));
+ testComparisonStdCheck(strncasecmp(left, right, n), SBuf(left).caseCmp(right, n));
+ testComparisonStdCheck(strncasecmp(left, right, n), SBuf(left).caseCmp(SBuf(right), n));
+}
+
+static void
+testComparisonStdOneWay(const char *left, const char *right) {
+ testComparisonStdFull(left, right);
+ const size_t maxN = 2 + min(strlen(left), strlen(right));
+ for (size_t n = 0; n <= maxN; ++n)
+ testComparisonStdN(left, right, n);
+}
+
+static void
+testComparisonStd(const char *s1, const char *s2) {
+ testComparisonStdOneWay(s1, s2);
+ testComparisonStdOneWay(s2, s1);
+}
+
void
testSBuf::testComparisons()
{
//same length
SBuf s1("foo"),s2("foe");
CPPUNIT_ASSERT(s1.cmp(s2)>0);
CPPUNIT_ASSERT(s1.caseCmp(s2)>0);
CPPUNIT_ASSERT(s2.cmp(s1)<0);
CPPUNIT_ASSERT_EQUAL(0,s1.cmp(s2,2));
CPPUNIT_ASSERT_EQUAL(0,s1.caseCmp(s2,2));
CPPUNIT_ASSERT(s1 > s2);
CPPUNIT_ASSERT(s2 < s1);
CPPUNIT_ASSERT_EQUAL(sign(s1.cmp(s2)),sign(strcmp(s1.c_str(),s2.c_str())));
//different lengths
s1.assign("foo");
s2.assign("foof");
CPPUNIT_ASSERT(s1.cmp(s2)<0);
CPPUNIT_ASSERT_EQUAL(sign(s1.cmp(s2)),sign(strcmp(s1.c_str(),s2.c_str())));
CPPUNIT_ASSERT(s1 < s2);
// specifying the max-length and overhanging size
CPPUNIT_ASSERT_EQUAL(1,SBuf("foolong").caseCmp(SBuf("foo"), 5));
// case-insensive comaprison
s1 = "foo";
s2 = "fOo";
CPPUNIT_ASSERT_EQUAL(0,s1.caseCmp(s2));
CPPUNIT_ASSERT_EQUAL(0,s1.caseCmp(s2,2));
// \0-clenliness test
s1.assign("f\0oo",4);
s2.assign("f\0Oo",4);
CPPUNIT_ASSERT(s1.cmp(s2) > 0);
CPPUNIT_ASSERT_EQUAL(0,s1.caseCmp(s2));
CPPUNIT_ASSERT_EQUAL(0,s1.caseCmp(s2,3));
CPPUNIT_ASSERT_EQUAL(0,s1.caseCmp(s2,2));
CPPUNIT_ASSERT_EQUAL(0,s1.cmp(s2,2));
+
+ testComparisonStd("foo", "fooz");
+ testComparisonStd("foo", "foo");
+ testComparisonStd("foo", "f");
+ testComparisonStd("foo", "bar");
+
+ testComparisonStd("foo", "FOOZ");
+ testComparisonStd("foo", "FOO");
+ testComparisonStd("foo", "F");
}
void
testSBuf::testConsume()
{
SBuf s1(literal),s2,s3;
s2=s1.consume(4);
s3.assign("The ");
CPPUNIT_ASSERT_EQUAL(s2,s3);
s3.assign("quick brown fox jumped over the lazy dog");
CPPUNIT_ASSERT_EQUAL(s1,s3);
s1.consume(40);
CPPUNIT_ASSERT_EQUAL(s1,SBuf());
}
void
testSBuf::testRawContent()
{
SBuf s1(literal);
SBuf s2(s1);
