[FileCheck] Simplify numeric variable interface

[thopre]

Summary

This patch simplifies 2 aspects in the FileCheckNumericVariable code.

First, setValue() method is turned into a void function since being
called only on undefined variable is an invariant and is now asserted
rather than returned. This remove the assert from the callers.

Second, clearValue() method is also turned into a void function since
the only caller does not check its return value since it may be trying
to clear the value of variable that is already cleared without this
being noteworthy.

[jhenderson]

llvm/include/llvm/Support/FileCheck.h

74–76

This phrasing feels a bit weird to me. How about:

"Sets value of this numeric variable, if undefined. The function will trigger an assertion failure if the variable is actually defined."

78–80

irregardless -> regardless

llvm/lib/Support/FileCheck.cpp

28

perhaps add "is not allowed".

[thopre]

Address review comments

[jhenderson]

llvm/lib/Support/FileCheck.cpp

28

This hasn't been addressed?

[thopre]

Rebase on top of latest changes in patch series

[thopre]

Address last comment

llvm/lib/Support/FileCheck.cpp

28

Oops, dunno how I missed it.

[jhenderson]

LGTM.

[probinson]

I was under the impression that an assertion in the public interface of a library API was not a great idea.

[thopre]

In D64231#1573921, @probinson wrote:

I was under the impression that an assertion in the public interface of a library API was not a great idea.

Oops my bad, I forgot about that comment. I think only what's in the FileCheck class should be considered public API. The rest ought to be moved to a different header file and is private. FileCheckNumericVariable is an implementation details IMO.

[probinson]

In D64231#1578086, @thopre wrote:

In D64231#1573921, @probinson wrote:

I was under the impression that an assertion in the public interface of a library API was not a great idea.

Oops my bad, I forgot about that comment. I think only what's in the FileCheck class should be considered public API. The rest ought to be moved to a different header file and is private. FileCheckNumericVariable is an implementation details IMO.

OK that's fair, can be done later.

llvm/include/llvm/Support/FileCheck.h

FileCheckNumericVariable(StringRef Name, uint64_t Value,
       : Name(Name), Value(Value), AvailLineNumber(AvailLineNumber) {}
 
   /// \returns name of this numeric variable.
   StringRef getName() const { return Name; }
 
   /// \returns this variable's value.
   Optional<uint64_t> getValue() const { return Value; }
 
-  /// Sets value of this numeric variable if not defined. \returns whether the
-  /// variable was already defined.
-  bool setValue(uint64_t Value);
-
-  /// Clears value of this numeric variable. \returns whether the variable was
-  /// already undefined.
-  bool clearValue();
+  /// Sets value of this numeric variable, if undefined. Triggers an assertion
+  /// failure if the variable is actually defined.
+  void setValue(uint64_t Value);
+
+  /// Clears value of this numeric variable, regardless of whether it is
+  /// currently defined or not.
+  void clearValue();
 
   /// \returns the line number where this variable's value becomes available.
   size_t getAvailLineNumber() { return AvailLineNumber; }
 };
 
 /// Type of functions evaluating a given binary operation.
 using binop_eval_t = uint64_t (*)(uint64_t, uint64_t);
 

llvm/lib/Support/FileCheck.cpp

 #include <cstdint>
 #include <list>
 #include <map>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 
-bool FileCheckNumericVariable::setValue(uint64_t NewValue) {
-  if (Value)
-    return true;
+void FileCheckNumericVariable::setValue(uint64_t NewValue) {
+  assert(!Value && "Overwriting numeric variable's value is not allowed");
   Value = NewValue;
-  return false;
 }
 
-bool FileCheckNumericVariable::clearValue() {
+void FileCheckNumericVariable::clearValue() {
   if (!Value)
-    return true;
+    return;
   Value = None;
-  return false;
 }
 
 Expected<uint64_t> FileCheckExpression::eval() const {
   assert(LeftOp && "Evaluating an empty expression");
   Optional<uint64_t> LeftOpValue = LeftOp->getValue();
   // Variable is undefined.
   if (!LeftOpValue)
     return make_error<FileCheckUndefVarError>(LeftOp->getName());
for (const auto &NumericVariableDef : NumericVariableDefs) {
     FileCheckNumericVariable *DefinedNumericVariable =
         NumericVariableMatch.DefinedNumericVariable;
 
     StringRef MatchedValue = MatchInfo[CaptureParenGroup];
     uint64_t Val;
     if (MatchedValue.getAsInteger(10, Val))
       return FileCheckErrorDiagnostic::get(SM, MatchedValue,
                                            "Unable to represent numeric value");
-    if (DefinedNumericVariable->setValue(Val))
-      llvm_unreachable("Numeric variable redefined");
+    DefinedNumericVariable->setValue(Val);
   }
 
   // Like CHECK-NEXT, CHECK-EMPTY's match range is considered to start after
   // the required preceding newline, which is consumed by the pattern in the
   // case of CHECK-EMPTY but not CHECK-NEXT.
   size_t MatchStartSkip = CheckTy == Check::CheckEmpty;
   MatchLen = FullMatch.size() - MatchStartSkip;
   return FullMatch.data() - Buffer.data() + MatchStartSkip;

llvm/unittests/Support/FileCheckTest.cpp

 #include "gtest/gtest.h"
 
 using namespace llvm;
 namespace {
 
 class FileCheckTest : public ::testing::Test {};
 
 TEST_F(FileCheckTest, NumericVariable) {
-  // Undefined variable: getValue and clearValue fails, setValue works.
+  // Undefined variable: getValue fails, setValue does not trigger assert.
   FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 1);
   EXPECT_EQ("FOO", FooVar.getName());
   llvm::Optional<uint64_t> Value = FooVar.getValue();
   EXPECT_FALSE(Value);
-  EXPECT_TRUE(FooVar.clearValue());
-  EXPECT_FALSE(FooVar.setValue(42));
+  FooVar.clearValue();
+  FooVar.setValue(42);
 
-  // Defined variable: getValue returns value set, setValue fails.
-  Value = FooVar.getValue();
-  EXPECT_TRUE(Value);
-  EXPECT_EQ(42U, *Value);
-  EXPECT_TRUE(FooVar.setValue(43));
+  // Defined variable: getValue returns value set.
   Value = FooVar.getValue();
   EXPECT_TRUE(Value);
   EXPECT_EQ(42U, *Value);
 
-  // Clearing variable: getValue fails, clearValue again fails.
-  EXPECT_FALSE(FooVar.clearValue());
+  // Clearing variable: getValue fails.
+  FooVar.clearValue();
   Value = FooVar.getValue();
   EXPECT_FALSE(Value);
-  EXPECT_TRUE(FooVar.clearValue());
 }
 
 uint64_t doAdd(uint64_t OpL, uint64_t OpR) { return OpL + OpR; }
 
 static void expectUndefError(const Twine &ExpectedStr, Error Err) {
   handleAllErrors(std::move(Err), [&](const FileCheckUndefVarError &E) {
     EXPECT_EQ(ExpectedStr.str(), E.getVarName());
   });