From: Francesco Chemolli Date: Mon, 1 Apr 2013 17:31:11 +0000 (+0200) Subject: Changes as per Mar 30th review by Amos X-Git-Tag: SQUID_3_5_0_1~612^2~18 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=496a9e97b8b9cbabbcd1c14091a51ce567e577e3;p=thirdparty%2Fsquid.git Changes as per Mar 30th review by Amos --- diff --git a/src/OutOfBoundsException.h b/src/OutOfBoundsException.h index 44fd4b08e9..5eb95465ae 100644 --- a/src/OutOfBoundsException.h +++ b/src/OutOfBoundsException.h @@ -15,8 +15,8 @@ public: virtual ~OutOfBoundsException() throw(); protected: - SBuf _buf; - SBuf::size_type _pos; + SBuf theThrowingBuf; + SBuf::size_type accessedPosition; }; #endif /* _SQUID_SRC_OUTOFBOUNDSEXCEPTION_H */ diff --git a/src/SBuf.cc b/src/SBuf.cc index 7ed1341cc6..207612be90 100644 --- a/src/SBuf.cc +++ b/src/SBuf.cc @@ -125,6 +125,16 @@ SBuf::SBuf(const String &S) ++stats.live; } +SBuf::SBuf(const std::string &s) + : store_(GetStorePrototype()), off_(0), len_(0) +{ + debugs(24, 8, id << " created from string"); + assign(s.data(),0,s.size()); + ++stats.alloc; + ++stats.allocFromString; + ++stats.live; +} + SBuf::SBuf(const char *S, size_type pos, size_type n) : store_(GetStorePrototype()), off_(0), len_(0) { @@ -276,7 +286,6 @@ SBuf::vappendf(const char *fmt, va_list vargs) Must(fmt != NULL); //we can assume that we'll need to append at least strlen(fmt) bytes, - //times 1.2 for instance... reserveSpace(strlen(fmt)*2); while (length() <= maxSize) { @@ -295,8 +304,10 @@ SBuf::vappendf(const char *fmt, va_list vargs) /* snprintf on Linux returns -1 on overflows */ /* snprintf on FreeBSD returns at least free_space on overflows */ - if (sz < 0 || sz >= (int)store_->spaceSize()) + if (sz >= static_cast(store_->spaceSize())) reserveSpace(sz*2); // TODO: tune heuristics + else if (sz < 0) // output error in vsnprintf + throw TextException("output error in vsnprintf",__FILE__, __LINE__); else break; } @@ -307,7 +318,7 @@ SBuf::vappendf(const char *fmt, va_list vargs) /* on XXX it might be counted */ /* check that '\0' is appended and not counted */ - if (operator[](len_-1) == 0) { + if (operator[](len_-1) == '\0') { --sz; --len_; } @@ -353,10 +364,10 @@ int SBuf::compare(const SBuf &S, SBufCaseSensitive isCaseSensitive, size_type n) const { Must(n == npos || n >= 0); + ++stats.compareSlow; size_type sz = min(S.length(), length()); if (n != npos) sz = min(n, sz); - ++stats.compareSlow; int rv; if (isCaseSensitive == caseSensitive) rv = strncmp(buf(), S.buf(), sz); @@ -374,7 +385,10 @@ SBuf::compare(const SBuf &S, SBufCaseSensitive isCaseSensitive, size_type n) con bool SBuf::startsWith(const SBuf &S, SBufCaseSensitive isCaseSensitive) const { + debugs(24, 8, id << " startsWith " << S.id << ", caseSensitive: " << + isCaseSensitive); if (length() < S.length()) { + debugs(24, 8, "no, too short"); ++stats.compareFast; return false; } @@ -431,12 +445,8 @@ SBuf::copy(char *dest, SBuf::size_type n) const { Must(n >= 0); - SBuf::size_type toexport = length(); - if (toexport > n) - toexport = n; - + SBuf::size_type toexport = min(n,length()); memcpy(dest, buf(), toexport); - ++stats.copyOut; return toexport; } @@ -480,14 +490,11 @@ SBuf::c_str() SBuf& SBuf::chop(SBuf::size_type pos, SBuf::size_type n) { - Must(pos >= 0); - Must(n == npos || n >= 0); - /* - * TODO: possible optimization: if the SBuf is at the tail of the - * MemBlob we could decrease the MemBlob tail-pointer so that a subsequent - * append will reuse the freed space. - */ - if (pos > length() || n == 0) { + if (pos != npos && pos < 0) + pos = 0; + if (n != npos && n < 0) + n = npos; + if (pos == npos || pos > length() || n == 0) { clear(); return *this; } @@ -669,7 +676,7 @@ SBuf::rfind(char c, SBuf::size_type endPos) const // NP: off-by-one weirdness: // endPos is an offset ... 0-based // length() is a count ... 1-based - // memrhr() requires a 1-based count of space to scan. + // memrchr() requires a 1-based count of space to scan. ++endPos; } @@ -730,12 +737,6 @@ SBuf::scanf(const char *format, ...) return rv; } -std::ostream & -operator <<(std::ostream& os, const SBuf& S) -{ - return S.print(os); -} - std::ostream & SBufStats::dump(std::ostream& os) const { @@ -808,7 +809,7 @@ SBuf::checkAccessBounds(SBuf::size_type pos) const { if (pos < 0) throw OutOfBoundsException(*this, pos, __FILE__, __LINE__); - if (pos > length()) + if (pos >= length()) throw OutOfBoundsException(*this, pos, __FILE__, __LINE__); } @@ -821,8 +822,8 @@ SBuf::toString() const return rv; } -/* - * re-allocate the backing store of the SBuf. +/** re-allocate the backing store of the SBuf. + * * If there are contents in the SBuf, they will be copied over. * NO verifications are made on the size parameters, it's up to the caller to * make sure that the new size is big enough to hold the copied contents. diff --git a/src/SBuf.cci b/src/SBuf.cci index 714b85102e..97ea13db86 100644 --- a/src/SBuf.cci +++ b/src/SBuf.cci @@ -29,6 +29,7 @@ */ #include "base/RefCount.h" +#include "Debug.h" #include "OutOfBoundsException.h" #include "SBufExceptions.h" @@ -100,6 +101,8 @@ SBuf::buf() const /** returns the pointer to the first char after this SBuf end * + * No checks are made that the space returned is safe, checking that is + * up to the caller. */ char * SBuf::bufEnd() const @@ -178,8 +181,6 @@ const char SBuf::operator [](SBuf::size_type pos) const { ++stats.getChar; - if (pos < 0 || pos >= length()) - return '\0'; return store_->mem[off_+pos]; } @@ -195,3 +196,9 @@ SBuf::isEmpty() const { return (len_ == 0); } + +std::ostream & +operator <<(std::ostream& os, const SBuf& S) +{ + return S.print(os); +} diff --git a/src/SBuf.h b/src/SBuf.h index 33fccfaf26..0563c8736f 100644 --- a/src/SBuf.h +++ b/src/SBuf.h @@ -32,7 +32,6 @@ #define SQUID_SBUF_H #include "base/InstanceId.h" -#include "Debug.h" #include "MemBlob.h" #include "SquidString.h" @@ -46,7 +45,7 @@ #include #endif -/* squid string placeholder (for printf) */ +/* SBuf placeholder for printf */ #ifndef SQUIDSBUFPH #define SQUIDSBUFPH "%.*s" #define SQUIDSBUFPRINT(s) (s).plength(),(s).rawContent() @@ -90,9 +89,7 @@ public: u_int64_t cowSlow; ///n - * bytes starting from position pos first byte is at pos 0. + * Removes SBuf prefix and suffix, leaving a sequence of 'n' + * bytes starting from position 'pos', first byte is at pos 0. + * It is an in-place-modifying version of substr. * \param pos start sub-stringing from this byte. If it is - * greater than the SBuf length, the SBuf is emptied and - * an empty SBuf is returned + * npos or it is greater than the SBuf length, the SBuf is cleared and + * an empty SBuf is returned. If it is <0, it is ignored * \param n maximum number of bytes of the resulting SBuf. * SBuf::npos means "to end of SBuf". - * if 0 returns an empty SBuf. + * if it is 0, the SBuf is cleared and an empty SBuf is returned. + * if it is < 0, it is ignored (same as supplying npos) + * if it overflows the end of the SBuf, it is capped to the end of SBuf + * \see substr, trim */ SBuf& chop(size_type pos, size_type n = npos); @@ -443,8 +442,9 @@ public: /** Extract a part of the current SBuf. * - * Return a fresh a fresh copy of a portion the current SBuf, which is left untouched. - * \see trim + * Return a fresh a fresh copy of a portion the current SBuf, which is + * left untouched. The same parameter convetions apply as for chop. + * \see trim, chop */ SBuf substr(size_type pos, size_type n = npos) const; @@ -453,7 +453,7 @@ public: * Returns the index in the SBuf of the first occurrence of char c. * \return SBuf::npos if the char was not found * \param startPos if specified, ignore any occurrences before that position - * if startPos is SBuf::npos, npos is always returned + * if startPos is npos or greater than length() npos is always returned * if startPos is < 0, it is ignored */ size_type find(char c, size_type startPos = 0) const; @@ -463,7 +463,7 @@ public: * Returns the index in the SBuf of the first occurrence of the * sequence contained in the str argument. * \param startPos if specified, ignore any occurrences before that position - * if startPos is SBuf::npos, npos is always returned + * if startPos is npos or greater than length() npos is always returned * if startPos is < 0, it is ignored * \return SBuf::npos if the SBuf was not found */ @@ -474,8 +474,8 @@ public: * Returns the index in the SBuf of the last occurrence of char c. * \return SBuf::npos if the char was not found * \param endPos if specified, ignore any occurrences after that position. - * if unspecified or npos, the whole SBuf is considered. - * If < 0, npos is returned + * if npos or greater than length(), the whole SBuf is considered + * if < 0, npos is always returned */ size_type rfind(char c, size_type endPos = npos) const; @@ -485,8 +485,8 @@ public: * sequence contained in the str argument. * \return SBuf::npos if the sequence was not found * \param endPos if specified, ignore any occurrences after that position - * if unspecified or npos, the whole SBuf is considered - * if < 0, then npos is always returned + * if npos or greater than length(), the whole SBuf is considered + * if < 0, npos is always returned */ size_type rfind(const SBuf &str, size_type endPos = npos) const; @@ -524,12 +524,13 @@ public: SBuf toUpper() const; /** String export function - * converts the SBuf to a legacy String, by copy. Transitional. + * converts the SBuf to a legacy String, by copy. + * \deprecated */ String toString() const; - /// TODO: possibly implement erase() similar to std::string's erase - /// TODO: possibly implement a replace() call + // TODO: possibly implement erase() similar to std::string's erase + // TODO: possibly implement a replace() call private: MemBlob::Pointer store_; ///< memory block, possibly shared with other SBufs @@ -554,10 +555,8 @@ private: }; -/** - * Prints a SBuf to the supplied stream, allowing for chaining - */ -std::ostream& operator <<(std::ostream &os, const SBuf &S); +/// ostream output operator +_SQUID_INLINE_ std::ostream& operator <<(std::ostream &os, const SBuf &S); #if _USE_INLINE_ #include "SBuf.cci" diff --git a/src/SBufExceptions.cc b/src/SBufExceptions.cc index 89dab78dfe..89c9f24fc5 100644 --- a/src/SBufExceptions.cc +++ b/src/SBufExceptions.cc @@ -33,19 +33,17 @@ #include "SBuf.h" #include "SBufExceptions.h" -// Note: the SBuf is intentionally passed by copy rather than reference, -// to let refcounting act. OutOfBoundsException::OutOfBoundsException(const SBuf &throwingBuf, SBuf::size_type &pos, const char *aFileName, int aLineNo) - :TextException(NULL, aFileName, aLineNo) + : TextException(NULL, aFileName, aLineNo), + theThrowingBuf(throwingBuf), + accessedPosition(pos) { - _buf = throwingBuf; - _pos = pos; SBuf explanatoryText("OutOfBoundsException"); if (aLineNo != -1) explanatoryText.appendf(" at line %d", aLineNo); - if (aFileName != 0) + if (aFileName != NULL) explanatoryText.appendf(" in file %s", aFileName); explanatoryText.appendf(" while accessing position %d in a SBuf long %d", pos, throwingBuf.length()); diff --git a/src/SBufStream.h b/src/SBufStream.h index fc6044e6fe..6378912764 100644 --- a/src/SBufStream.h +++ b/src/SBufStream.h @@ -36,15 +36,16 @@ #include #endif -/** - * streambuf class for a SBuf-backed stream interface. +/** streambuf class for a SBuf-backed stream interface. * + * Auxiliary class to be able to leverage an ostream generating SBuf's + * analogous to std::ostrstream. */ class SBufStreamBuf : public std::streambuf { public: /// initialize streambuf; use supplied SBuf as backing store - SBufStreamBuf(SBuf aBuf) : theBuf(aBuf) {} + explicit SBufStreamBuf(SBuf aBuf) : theBuf(aBuf) {} /// get a copy of the stream's contents SBuf getBuf() { @@ -74,7 +75,7 @@ protected: return aChar; } - /* push the streambuf to the backing SBuf */ + /// push the streambuf to the backing SBuf virtual int sync() { std::streamsize pending(pptr() - pbase()); @@ -84,8 +85,8 @@ protected: return 0; } - /* write multiple characters to the store entry - * - this is an optimisation method. + /** write multiple characters to the store entry + * \note this is an optimisation consistent with std::streambuf API */ virtual std::streamsize xsputn(const char * chars, std::streamsize number) { if (number) @@ -107,10 +108,10 @@ private: class SBufStream : public std::ostream { public: - /** Create a SBufStream preinitialized with the argument's SBuf. + /** Create a SBufStream preinitialized with the contents of a SBuf * - * The supplied SBuf is not aliased: in order to retrieve the altered contents - * they must be fetched using the buf() class method. + * The supplied SBuf copied: in order to retrieve the written-to contents + * they must be later fetched using the buf() class method. */ SBufStream(SBuf aBuf): std::ostream(0), theBuffer(aBuf) { rdbuf(&theBuffer); // set the buffer to now-initialized theBuffer diff --git a/src/tests/testSBuf.cc b/src/tests/testSBuf.cc index 559209a837..a01aaaafd1 100644 --- a/src/tests/testSBuf.cc +++ b/src/tests/testSBuf.cc @@ -111,6 +111,15 @@ testSBuf::testSBufConstructDestruct() SBuf s1(str); CPPUNIT_ASSERT_EQUAL(s1,literal); } + + // TEST: go via std::string adapter. + { + std::string str(fox); + SBuf s1(str); + CPPUNIT_ASSERT_EQUAL(s1,literal); + } + + } void @@ -119,9 +128,6 @@ testSBuf::testSBufConstructDestructAfterMemInit() Mem::Init(); testSBufConstructDestruct(); -// XXX: or perhapse ... -// repeat all of the tests inside testSBufConstructDestructBeforeMemInit() -// with additional checks on Mem usage stats after each operation ?? } void @@ -202,7 +208,6 @@ testSBuf::testSubscriptOp() chg.setAt(5,'e'); CPPUNIT_ASSERT_EQUAL(literal[5],'u'); CPPUNIT_ASSERT_EQUAL(chg[5],'e'); -// std::cout << chg << std::endl << empty_sbuf << std::endl ; } // note: can't use cppunit's CPPUNIT_TEST_EXCEPTION because TextException asserts, and @@ -211,7 +216,7 @@ void testSBuf::testSubscriptOpFail() { char c; - c=literal.at(1234); //out of bounds + c=literal.at(literal.length()); //out of bounds by 1 //notreached std::cout << c << std::endl; } @@ -274,8 +279,10 @@ testSBuf::testRawSpace() { SBuf s1(literal); SBuf s2(fox1); + SBuf::size_type sz=s2.length(); char *rb=s2.rawSpace(strlen(fox2)+1); - strcat(rb,fox2); + strcpy(rb,fox2); + s2.forceSize(sz+strlen(fox2)); CPPUNIT_ASSERT_EQUAL(s1,s2); } @@ -290,6 +297,57 @@ testSBuf::testChop() s2.clear(); s1.chop(5,0); CPPUNIT_ASSERT_EQUAL(s1,s2); + const char *alphabet="abcdefghijklmnopqrstuvwxyz"; + SBuf a(alphabet); + std::string s(alphabet); // TODO + { //regular chopping + SBuf b(a); + b.chop(3,3); + SBuf ref("def"); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // chop at end + SBuf b(a); + b.chop(b.length()-3); + SBuf ref("xyz"); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // chop at beginning + SBuf b(a); + b.chop(0,3); + SBuf ref("abc"); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // chop to zero length + SBuf b(a); + b.chop(5,0); + SBuf ref(""); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // chop beyond end (at npos) + SBuf b(a); + b.chop(SBuf::npos,4); + SBuf ref(""); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // chop beyond end + SBuf b(a); + b.chop(b.length()+2,4); + SBuf ref(""); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // null-chop + SBuf b(a); + b.chop(0,b.length()); + SBuf ref(a); + CPPUNIT_ASSERT_EQUAL(ref,b); + } + { // overflow chopped area + SBuf b(a); + b.chop(b.length()-3,b.length()); + SBuf ref("xyz"); + CPPUNIT_ASSERT_EQUAL(ref,b); + } } void @@ -308,6 +366,29 @@ testSBuf::testChomp() CPPUNIT_ASSERT_EQUAL(s1,s2); } +// inspired to SBufFindTest; to be expanded. +class SBufSubstrAutoTest +{ + SBuf fullString, sb; + std::string fullReference, str; + public: + void performEqualityTest() + { + SBuf ref(str); + CPPUNIT_ASSERT_EQUAL(ref,sb); + } + SBufSubstrAutoTest() : fullString(fox), fullReference(fox) + { + for (int offset=fullString.length()-1; offset >= 0; --offset ) { + for (int length=fullString.length()-1-offset; length >= 0; --length) { + sb=fullString.substr(offset,length); + str=fullReference.substr(offset,length); + performEqualityTest(); + } + } + } +}; + void testSBuf::testSubstr() { @@ -317,6 +398,7 @@ testSBuf::testSubstr() CPPUNIT_ASSERT_EQUAL(s2,s3); s1.chop(4,5); CPPUNIT_ASSERT_EQUAL(s1,s2); + SBufSubstrAutoTest sat; // work done in the constructor } void diff --git a/src/tests/testSBuf.h b/src/tests/testSBuf.h index de599736bc..241889f219 100644 --- a/src/tests/testSBuf.h +++ b/src/tests/testSBuf.h @@ -26,7 +26,7 @@ class testSBuf : public CPPUNIT_NS::TestFixture CPPUNIT_TEST( testComparisons ); CPPUNIT_TEST( testConsume ); CPPUNIT_TEST( testRawContent ); - //CPPUNIT_TEST( testRawSpace ); + CPPUNIT_TEST( testRawSpace ); CPPUNIT_TEST( testChop ); CPPUNIT_TEST( testChomp ); CPPUNIT_TEST( testSubstr ); @@ -42,9 +42,7 @@ class testSBuf : public CPPUNIT_NS::TestFixture CPPUNIT_TEST( testGrow ); CPPUNIT_TEST( testSBufStream ); CPPUNIT_TEST( testAutoFind ); - // CPPUNIT_TEST( testDumpStats ); //fake test, to print alloc stats - CPPUNIT_TEST_SUITE_END(); protected: void commonInit();