Minor fixes

include/xrpl/beast/utility/instrumentation.h

@@ -37,22 +37,27 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 #define ASSERT ALWAYS_OR_UNREACHABLE
 
-// How to use the above macros:
+// How to use the instrumentation macros:
 //
-// ALWAYS if it must be reached during fuzzing
-// ASSERT if you expect that it might, or might not, be reached during fuzzing
-// REACHABLE if it must be reached during fuzzing
-// SOMETIMES a hint for the fuzzer to try to make the condition true
-// UNREACHABLE if it must not be reached (in fuzzing or in normal use)
+// ALWAYS if cond must be true and the line must be reached during fuzzing
+// ASSERT if cond must be true but the line might not be reached during fuzzing
+// REACHABLE if the line must be reached during fuzzing
+// SOMETIMES a hint for the fuzzer to try to make the cond true
+// UNREACHABLE if the line must not be reached (in fuzzing or in normal use)
 //
-// NOTE: ASSERT has similar semantics as assert macro, with minor differences:
+// NOTE: ASSERT has similar semantics as C assert macro, with minor differences:
 // * ASSERT must have an unique name (naming convention in CONTRIBUTING.md)
 // * the condition (which comes first) must be *implicitly* convertible to bool
 // * during fuzzing, the program will continue execution past a failed ASSERT
-// We continue to use assert inside unit tests and in constexpr functions.
+//
+// We continue to use regular C assert inside unit tests and inside constexpr
+// functions.
 //
 // NOTE: UNREACHABLE does *not* have the same semantics as std::unreachable.
 // The program will continue execution past an UNREACHABLE in a Release build
-// and during fuzzing (similar to ASSERT).
+// and during fuzzing (similar to ASSERT). Also the naming convention is subtly
+// different from all other macros - name in UNREACHABLE describes the condition
+// which was meant to never happen, while name in other macros describe the
+// condition that is meant to happen (e.g. as in "assert that this happens").
 
 #endif

src/libxrpl/basics/contract.cpp

@@ -49,6 +49,9 @@ LogicError(std::string const& s) noexcept
 {
     JLOG(debugLog().fatal()) << s;
     std::cerr << "Logic error: " << s << std::endl;
+    // Use a non-standard contract naming here (without namespace), because
+    // it's the only location where various unrelated execution paths may
+    // register an error; this is also why "message" parameter is passed here.
     UNREACHABLE("LogicError", {{"message", s}});
     detail::accessViolation();
 }

src/libxrpl/json/Object.cpp

@@ -168,7 +168,7 @@ Array::append(Json::Value const& v)
             return;
         }
     }
-    UNREACHABLE("Json::Array::append : invalid type"); 
+    UNREACHABLE("Json::Array::append : invalid type");
 }
 
 void

src/libxrpl/json/json_reader.cpp

@@ -953,7 +953,7 @@ operator>>(std::istream& sin, Value& root)
     Json::Reader reader;
     bool ok = reader.parse(sin, root);
 
-    // ASSERT( ok );
+    // ASSERT(ok, "Json::operator>>() : parse succeeded");
     if (!ok)
         ripple::Throw<std::runtime_error>(reader.getFormatedErrorMessages());
 

src/libxrpl/protocol/Indexes.cpp

@@ -208,7 +208,7 @@ line(
     // There is code in SetTrust that calls us with id0 == id1, to allow users
     // to locate and delete such "weird" trustlines. If we remove that code, we
     // could enable this assert:
-    // ASSERT(id0 != id1);
+    // ASSERT(id0 != id1, "ripple::keylet::line : accounts must be different");
 
     // A trust line is shared between two accounts; while we typically think
     // of this as an "issuer" and a "holder" the relationship is actually fully

src/xrpld/app/paths/detail/StrandFlow.h

@@ -847,7 +847,7 @@ flow(
             // running debug builds of rippled. While this issue still needs to
             // be resolved, the assert is causing more harm than good at this
             // point.
-            // UNREACHABLE();
+            // UNREACHABLE("ripple::flow : rounding error");
 
             return {tefEXCEPTION, std::move(ofrsToRmOnFail)};
         }

src/xrpld/app/tx/detail/OfferStream.cpp

@@ -342,7 +342,7 @@ TOfferStreamBase<TIn, TOut>::step()
             }
             UNREACHABLE(
                 "rippls::TOfferStreamBase::step::rmSmallIncreasedQOffer : XRP "
-                "vs XRP offer");  // xrp/xrp offer!?! should never happen
+                "vs XRP offer");
             return false;
         }();
 

src/xrpld/app/tx/detail/SetSignerList.cpp

@@ -128,8 +128,7 @@ SetSignerList::doApply()
         default:
             break;
     }
-    UNREACHABLE(
-        "ripple::SetSignerList::doApply : invalid operation");
+    UNREACHABLE("ripple::SetSignerList::doApply : invalid operation");
     return temMALFORMED;
 }