From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 16 Mar 2021 06:23:02 +0000 (+0000)
Subject: Replace defective Must2(c, "not c") API (#785)
X-Git-Tag: 4.15-20210522-snapshot~23
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2bde9c7cec0d243d5b1e73b19da19ef0a5d8a08a;p=thirdparty%2Fsquid.git

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 a37e21a632..6f54918478 100755
--- 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 2a43fac358..5f7ce4732e 100644
--- 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 a7eb7a723f..8cd45bc861 100644
--- 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