Documentation improvements

CONTRIBUTING.md

@@ -385,13 +385,19 @@ For this reason:
   function name. NOTE: the purpose of name is to provide stable means of
   unique identification of every contract; for this reason try to avoid elements
   which can change in some obvious refactors or when reinforcing the condition.
+* Contract description typically (except for `UNREACHABLE`) should describe the
+  _expected_ condition, as in "I assert that _expected_ is true".
+* Contract description for `UNREACHABLE` should describe the _unexpected_
+  situation which caused the line to have been reached.
 * Example good name for an
   `UNREACHABLE` macro `"Json::operator==(Value, Value) : invalid type"`; example
   good name for an `ASSERT` macro `"Json::Value::asCString : valid type"`.
 * Example **bad** name
   `"RFC1751::insert(char* s, int x, int start, int length) : length is greater than or equal zero"`
   (missing namespace, unnecessary full function signature, description too verbose).
   Good name: `"ripple::RFC1751::insert : minimum length"`.
+* In **few** well-justified cases a non-standard name can be used, in which case a
+  comment should be placed to explain the rationale (example in `contract.cpp`)
 * Do **not** rename a contract without a good reason (e.g. the name no longer
   reflects the location or the condition being checked)
 * Do not use `std::unreachable`

include/xrpl/beast/utility/instrumentation.h

@@ -55,9 +55,10 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 //
 // 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). 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").
+// and during fuzzing (similar to ASSERT).
+// Also, the naming convention in UNREACHABLE is subtly different from other
+// instrumentation macros - its name describes the condition which was *not*
+// meant to 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,9 +49,11 @@ 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
+    // 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.
+    // register an error; this is also why the "message" parameter is passed
+    // here.
+    // For the above reasons, we want this contract to stand out.
     UNREACHABLE("LogicError", {{"message", s}});
     detail::accessViolation();
 }