Turn unreachable error into assert

[thopre]

Summary

Function valueFromStringRepr() throws an error on missing 0x prefix when
parsing a number string into a value. However, getWildcardRegex() already
ensures that only text with the 0x prefix will match and be parsed,
making that error throwing code dead code. This commit turn the code
into an assert and remove the unit tests exercising that test
accordingly.

[jhenderson]

Given that there are unit tests, this code is in the library's public-facing API. Does removing the error check cause problems if users happen to be leveraging the FileCheck library in some manner?

[thopre]

In D150797#4352145, @jhenderson wrote:

Given that there are unit tests, this code is in the library's public-facing API. Does removing the error check cause problems if users happen to be leveraging the FileCheck library in some manner?

It is in the library but not exposed in the FileCheck library header. The unit tests use the FileCheck internal header. The library interface is much higher level than that: it is essentially composed of the readCheckFile and checkInput functions. Everything else is internal.

[jhenderson]

llvm/lib/FileCheck/FileCheck.cpp

148–151

I'd get rid of the unnecessary temporary and just do the calculation inline.

Also, is this the right place for the assert? Previously the error was later in the code.

[thopre]

llvm/lib/FileCheck/FileCheck.cpp

148–151

Will the consume_front() happen if it's inside the assert though? Because under normal circumstances (i.e. assert does not trip) the consume_front() *must* happen for the function to do the right thing.

[jhenderson]

LGTM, with one nit.

llvm/lib/FileCheck/FileCheck.cpp

148–151

Bleh, I missed that consume_front has side effects beyond getting the return value. Okay, I can't think of any real improvements (other than I think static_cast is preferred generally to C-style casts).

[thopre]

llvm/lib/FileCheck/FileCheck.cpp

148–151

I agree in the general case but there is not a single use of static_cast<void> to indicate a function return value being ignored in the llvm/ subtree. There is however plenty of (void) C-style cast example. Ok to land as is?

[jhenderson]

Go ahead with landing. I'm going to start a discussion on Discourse about coding style for casts as I don't see it discussed anywhere in the coding standards.

llvm/lib/FileCheck/FileCheck.cpp

if (ValueIsSigned) {
     if (StrVal.getAsInteger(10, SignedValue))
       return ErrorDiagnostic::get(SM, StrVal, IntegerParseErrorStr);
 
     return ExpressionValue(SignedValue);
   }
 
   bool Hex = Value == Kind::HexUpper || Value == Kind::HexLower;
   uint64_t UnsignedValue;
   bool MissingFormPrefix = AlternateForm && !StrVal.consume_front("0x");
+  (void)MissingFormPrefix;
+  assert(!MissingFormPrefix && "missing alternate form prefix");
   if (StrVal.getAsInteger(Hex ? 16 : 10, UnsignedValue))
     return ErrorDiagnostic::get(SM, StrVal, IntegerParseErrorStr);
 
-  // Error out for a missing prefix only now that we know we have an otherwise
-  // valid integer.  For example, "-0x18" is reported above instead.
-  if (MissingFormPrefix)
-    return ErrorDiagnostic::get(SM, StrVal, "missing alternate form prefix");
-
   return ExpressionValue(UnsignedValue);
 }
 
 static int64_t getAsSigned(uint64_t UnsignedValue) {
   // Use memcpy to reinterpret the bitpattern in Value since casting to
   // signed is implementation-defined if the unsigned value is too big to be
   // represented in the signed type and using an union violates type aliasing
   // rules.

llvm/unittests/FileCheck/FileCheckTest.cpp

 TEST_P(ExpressionFormatParameterisedFixture, FormatValueFromStringRepr) {
   checkValueFromStringRepr(addBasePrefix("0"), 0);
   checkValueFromStringRepr(addBasePrefix("9"), 9);
 
   if (Signed) {
     checkValueFromStringRepr("-5", -5);
     checkValueFromStringReprFailure(MaxUint64Str);
   } else {
-    checkValueFromStringReprFailure("-" + addBasePrefix("5"));
     checkValueFromStringRepr(addBasePrefix(MaxUint64Str), MaxUint64);
   }
 
   checkValueFromStringRepr(addBasePrefix(TenStr), 10);
   checkValueFromStringRepr(addBasePrefix(FifteenStr), 15);
 
   // Wrong casing is not tested because valueFromStringRepr() relies on
   // StringRef's getAsInteger() which does not allow to restrict casing.
   checkValueFromStringReprFailure(addBasePrefix("G"));
-
-  if (AlternateForm)
-    checkValueFromStringReprFailure("9", "missing alternate form prefix");
 }
 
 TEST_P(ExpressionFormatParameterisedFixture, FormatBoolOperator) {
   EXPECT_TRUE(bool(Format));
 }
 
 INSTANTIATE_TEST_SUITE_P(
     AllowedExplicitExpressionFormat, ExpressionFormatParameterisedFixture,