From: Francesco Chemolli <kinkie@squid-cache.org>
Date: Wed, 31 Jul 2013 12:17:23 +0000 (+0200)
Subject: Improved SBuf::appendf() readability and correctness.
X-Git-Tag: SQUID_3_5_0_1~612^2~6
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8aa34dad397963a6153bb5ab4e8a4a7e1ca9b197;p=thirdparty%2Fsquid.git

Improved SBuf::appendf() readability and correctness.
Clarified documentation.
Added SBuf::spaceSize()
Fixed display result in appendf() unit test.
---

diff --git a/src/SBuf.cc b/src/SBuf.cc
index b49a40c5c4..64f1a82d69 100644
--- a/src/SBuf.cc
+++ b/src/SBuf.cc
@@ -268,47 +268,45 @@ SBuf::appendf(const char *fmt, ...)
 SBuf&
 SBuf::vappendf(const char *fmt, va_list vargs)
 {
+    Must(fmt != NULL);
+    int sz = 0;
+    //reserve twice the format-string size, it's a likely heuristic
+    size_type requiredSpaceEstimate = strlen(fmt)*2;
+
+    char *space = rawSpace(requiredSpaceEstimate);
 #ifdef VA_COPY
     va_list ap;
+    VA_COPY(ap, vargs);
+    sz = vsnprintf(space, spaceSize(), fmt, ap);
+    va_end(ap);
+#else
+    sz = vsnprintf(space, spaceSize(), fmt, vargs);
 #endif
-    int sz = 0;
 
-    Must(fmt != NULL);
-
-    //reserve twice the format-string size, it's a likely heuristic
-    rawSpace(strlen(fmt)*2);
+    /* check for possible overflow */
+    /* snprintf on Linux returns -1 on output errors, or the size
+     * that would have been written if enough space had been available */
+    /* vsnprintf is standard in C99 */
 
-    while (length() <= maxSize) {
-#ifdef VA_COPY
-        /* Fix of bug 753r. The value of vargs is undefined
-         * after vsnprintf() returns. Make a copy of vargs
-         * in case we loop around and call vsnprintf() again.
-         */
-        VA_COPY(ap, vargs);
-        sz = vsnprintf(bufEnd(), store_->spaceSize(), fmt, ap);
-        va_end(ap);
-#else /* VA_COPY */
-        sz = vsnprintf(bufEnd(), store_->spaceSize(), fmt, vargs);
-#endif /* VA_COPY*/
-        /* check for possible overflow */
-        /* snprintf on Linux returns -1 on overflows */
-        /* snprintf on FreeBSD returns at least free_space on overflows */
-
-        if (sz >= static_cast<int>(store_->spaceSize()))
-            rawSpace(sz*2); // TODO: tune heuristics
-        else if (sz < 0) // output error in vsnprintf
-            throw TextException("output error in vsnprintf",__FILE__, __LINE__);
-        else
-            break;
+    if (sz >= static_cast<int>(spaceSize())) {
+        // not enough space on the first go, we now know how much we need
+        requiredSpaceEstimate = sz*2; // TODO: tune heuristics
+        space = rawSpace(requiredSpaceEstimate);
+        sz = vsnprintf(space, spaceSize(), fmt, vargs);
     }
 
+    if (sz < 0) // output error in either vsnprintf
+        throw TextException("output error in vsnprintf",__FILE__, __LINE__);
+
+    // data was appended, update internal state
     len_ += sz;
+
     // TODO: this does NOT belong here, but to class-init or autoconf
-    /* on Linux and FreeBSD, '\0' is not counted in return value */
-    /* on XXX it might be counted */
-    /* check that '\0' is appended and not counted */
+    /* C99 specifies that the final '\0' is not counted in vsnprintf's
+     * return value. Older compilers/libraries might instead count it */
+    /* check whether '\0' was appended and counted */
 
-    if (operator[](len_-1) == '\0') {
+    if (at(len_-1) == '\0') {
         --sz;
         --len_;
     }
diff --git a/src/SBuf.h b/src/SBuf.h
index fb65ca6f58..91ae9ae34d 100644
--- a/src/SBuf.h
+++ b/src/SBuf.h
@@ -346,14 +346,20 @@ public:
      * be invalidated by the first call to a non-const method call
      * on the SBuf.
      * This call guarantees to never return NULL.
-     * This method can also be used to prepare the SBuf by preallocating a
-     * predefined amount of free space; this may help to optimize subsequent
-     * calls to SBuf::append or similar methods. In this case the returned
-     * pointer should be ignored.
+     * \see reserveSpace
+     * \note Unlike reserveSpace(), this method does not guarantee exclusive
+     *       buffer ownership. It is instead optimized for a one writer
+     *       (appender), many readers scenario by avoiding unnecessary
+     *       copying and allocations.
      * \throw SBufTooBigException if the user tries to allocate too big a SBuf
      */
     char *rawSpace(size_type minSize);
 
+    /**
+     *
+     */
+    size_type spaceSize() const { return store_->spaceSize(); }
+
     /** Force a SBuf's size
      * \warning use with EXTREME caution, this is a dangerous operation
      *
@@ -403,7 +409,7 @@ public:
      */
     bool isEmpty() const {return (len_==0);}
 
-    /** Request to extend the SBuf's free store space.
+    /** Request to guarantee the SBuf's free store space.
      *
      * After the reserveSpace request, the SBuf is guaranteed to have at
      * least minSpace bytes of unused backing store following the currently
@@ -412,7 +418,7 @@ public:
      */
     void reserveSpace(size_type minSpace) {reserveCapacity(length()+minSpace);}
 
-    /** Request to resize the SBuf's store capacity
+    /** Request to guarantee the SBuf's store capacity
      *
      * After this method is called, the SBuf is guaranteed to have at least
      * minCapacity bytes of total buffer size, including the currently-used
diff --git a/src/tests/testSBuf.cc b/src/tests/testSBuf.cc
index 41cc334a3b..432093a6f1 100644
--- a/src/tests/testSBuf.cc
+++ b/src/tests/testSBuf.cc
@@ -201,7 +201,7 @@ testSBuf::testAppendf()
     SBuf s1,s2;
     s1.appendf("%s:%d:%03.2f",fox,1234,1234.56);
     s2.assign("The quick brown fox jumped over the lazy dog:1234:1234.56");
-    CPPUNIT_ASSERT_EQUAL(s1,s2);
+    CPPUNIT_ASSERT_EQUAL(s2,s1);
 }
 
 void