Replace defective Must2(c, "not c") API (#785)

... with Must3(c, "c", Here())

* We should not make a Must*() API different from the standard C++
  static_assert() API. It is just too confusing, especially since the
  two calls are very closely related.

* We should not tell Must*() writers to specify one condition (i.e. what
  must happen) but then describe the opposite condition (i.e. what went
  wrong). When the text describes the opposite condition, it is easy for
  a human writing or thinking about the text to type the wrong
  condition: Must2(got < need, "incomplete message") looks correct!

We should not keep the same macro name when changing the meaning of the
second parameter. Fortunately, adding a third argument to the macro fits
nicely into how modern Squid code should pass the source code location.

Must3() does not support SBuf descriptions. I tried to support them, but
doing so without duplicating non-trivial code is too difficult, and
since the current code does not actually use them, wasteful.

If future (complex) code needs SBuf conditions, then it should probably
use an explicit throw statement instead, especially since Must*() is,
like assert(), supposed to quickly check source code sanity rather than
validate unsafe input. A compile-time condition description and the
source code location is usually enough for sanity checks, while proper
reporting of invalid input usually also requires parsing context info.

diff --git a/scripts/calc-must-ids.pl b/scripts/calc-must-ids.pl
index a37e21a63261469d51239e0ba0ab99feb3815237..6f54918478eff26a34f3e5822f1769949bec6cb9 100755 (executable)
--- a/scripts/calc-must-ids.pl
+++ b/scripts/calc-must-ids.pl
@@ -66,7 +66,7 @@ sub ComputeMustIds
         $line =~ s@/[*].*?[*]/@@; # strip simple single-line /* comments */
 
         my($id);
-        if ($line =~ /\bMust2?\s*\(/ || # Must(...) and Must2(...)
+        if ($line =~ /\bMust\s*\(/ || # Must(...)
             $line =~ /\bTexcHere\s*\(/ || # TexcHere(...)
             $line =~ /\bHere\s*\(\s*\)/) { # Here()
             $line =~ s/^\s*//;

diff --git a/src/base/TextException.h b/src/base/TextException.h
index 2a43fac358e3d85cf8e35378ef9ccd2bd9587298..5f7ce4732e4e8019d4cd32b6aad8c4031a1f8727 100644 (file)
--- a/src/base/TextException.h
+++ b/src/base/TextException.h
@@ -58,18 +58,19 @@ std::ostream &operator <<(std::ostream &, const TextException &);
 #define TexcHere(msg) TextException((msg), Here())
 
 /// Like assert() but throws an exception instead of aborting the process
-/// and allows the caller to specify a custom exception message.
-#define Must2(condition, message) \
+/// and allows the caller to customize the exception message and location.
+/// \param description string literal describing the condition; what MUST happen
+#define Must3(condition, description, location) \
     do { \
         if (!(condition)) { \
-            const TextException Must_ex_((message), Here()); \
+            const TextException Must_ex_(("check failed: " description), (location)); \
             debugs(0, 3, Must_ex_.what()); \
             throw Must_ex_; \
         } \
     } while (/*CONSTCOND*/ false)
 
 /// Like assert() but throws an exception instead of aborting the process.
-#define Must(condition) Must2((condition), "check failed: " #condition)
+#define Must(condition) Must3((condition), #condition, Here())
 
 /// Reports and swallows all exceptions to prevent compiler warnings and runtime
 /// errors related to throwing class destructors. Should be used for most dtors.

diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc
index a7eb7a723f40755d1746db37dd96c041b9fb9bf6..8cd45bc861caaa9dc95e8849b67452295827acd3 100644 (file)
--- a/src/sbuf/SBuf.cc
+++ b/src/sbuf/SBuf.cc
@@ -148,7 +148,7 @@ SBuf::rawAppendFinish(const char *start, size_type actualSize)
     debugs(24, 8, id << " finish appending " << actualSize << " bytes");
 
     size_type newSize = length() + actualSize;
-    Must2(newSize <= min(maxSize,store_->capacity-off_), "raw append overflow");
+    Must3(newSize <= min(maxSize, store_->capacity-off_), "raw append fits", Here());
     len_ = newSize;
     store_->size = off_ + newSize;
 }
@@ -258,7 +258,7 @@ SBuf::vappendf(const char *fmt, va_list vargs)
     va_copy(ap, vargs);
     sz = vsnprintf(space, spaceSize(), fmt, ap);
     va_end(ap);
-    Must2(sz >= 0, "vsnprintf() output error");
+    Must3(sz >= 0, "vsnprintf() succeeds", Here());
 
     /* check for possible overflow */
     /* snprintf on Linux returns -1 on output errors, or the size
@@ -270,7 +270,7 @@ SBuf::vappendf(const char *fmt, va_list vargs)
         requiredSpaceEstimate = sz*2; // TODO: tune heuristics
         space = rawSpace(requiredSpaceEstimate);
         sz = vsnprintf(space, spaceSize(), fmt, vargs);
-        Must2(sz >= 0, "vsnprintf() output error despite increased buffer space");
+        Must3(sz >= 0, "vsnprintf() succeeds (with increased buffer space)", Here());
     }
 
     // data was appended, update internal state