[PATCH] D60387: FileCheck [7/12]: Arbitrary long numeric expressions

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks good. A few nits still and one or two more significant comments. I think this structure looks much cleaner than the previous version, personally.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:54
+/// Class representing a literal in the AST of an expression.
+class FileCheckExpressionLiteral : public FileCheckExpressionAST {
+private:
----------------
As things stand, this only can represent an unsigned literal. I think you'll want to support negative literals or unary negation operands fairly quickly. Otherwise, the expression "-1" doesn't work.

I'm also slightly concerned in the current form that somebody could try to use FileCheckExpressionLiteral for negative numbers, and get surprising results. Perhaps worth updating the comment to say that the class represents an unsigned integer literal.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:60
+public:
+  /// Constructor for a literal.
+  FileCheckExpressionLiteral(uint64_t Val) : Value(Val) {}
----------------
Perhaps "Construct a literal with the specified value."


================
Comment at: llvm/lib/Support/FileCheck.cpp:49
+
+  // Uses undefined variable(s).
+  if (!LeftOp || !RightOp) {
----------------
Might be worth making this comment more generic e.g. "Handle a failed operation, e.g. due to an undefined variable." That'll make the comment robust to other possible future failure types.


================
Comment at: llvm/lib/Support/FileCheck.cpp:258
                                          "missing operand in expression");
-  uint64_t RightOp;
-  if (Expr.consumeInteger(10, RightOp))
-    return FileCheckErrorDiagnostic::get(
-        SM, Expr, "invalid offset in expression '" + Expr + "'");
-  Expr = Expr.ltrim(SpaceChars);
-  if (!Expr.empty())
-    return FileCheckErrorDiagnostic::get(
-        SM, Expr, "unexpected characters at end of expression '" + Expr + "'");
+  // Second operand in a legacy @LINE expression is a literal.
+  AllowedOperand AO =
----------------
is always a literal

The comment should probably start with "The second..."


================
Comment at: llvm/lib/Support/FileCheck.cpp:301
   Expr = Expr.ltrim(SpaceChars);
-  return parseBinop(Expr, SM);
+  // First operand in a legacy @LINE expression is the @LINE pseudo variable.
+  AllowedOperand AO =
----------------
The comment should probably start with "The first..."


================
Comment at: llvm/lib/Support/FileCheck.cpp:722
                           if (!UndefSeen) {
-                            OS << "uses undefined variable ";
+                            OS << "uses undefined variable(s):";
                             UndefSeen = true;
----------------
You've changed this message implying it can have more than one undefined variable, but I don't see any lit test case change that has more than one. You should add/update one.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:17
 
+TEST_F(FileCheckTest, Literal) {
+  // Eval returns the literal's value.
----------------
I think it would be a good idea to have a test case showing that this can take std::numeric_limits<uint64_t>::max().


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:25
+
+static void expectUndefErrors(std::set<std::string> ExpectedUndefVarNames,
+                              Error Err) {
----------------
`const std::set &`?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:30
+  });
+  EXPECT_TRUE(ExpectedUndefVarNames.empty());
+}
----------------
Perhaps worth having this print what names are still remaining if not empty. I think you can do something like `EXPECT_TRUE(ExpectedUndefVarNames.empty()) << ExpectedUndefVarNames`.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:50
 
-  // Defined variable: getValue returns value set.
-  Value = FooVar.getValue();
-  EXPECT_TRUE(Value);
+  // Defined variable: getvalue and eval return value set.
+  Optional<uint64_t> Value = FooVar.getValue();
----------------
getValue not getvalue.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:397
+  SubstValue = SubstitutionN.getResult();
+  EXPECT_TRUE(static_cast<bool>(SubstValue));
+  EXPECT_EQ("10", *SubstValue);
----------------
In other places you've used `bool(SubstValue)` for this sort of conversion. I think that might be nicer, but don't care that much. Regardless, it should be consistent throughout.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:479
   Optional<FileCheckNumericVariable *> DefinedNumericVariable;
-  Expected<FileCheckExpression *> Expression = P.parseNumericSubstitutionBlock(
-      LocalNumVarRef, DefinedNumericVariable, SM);
-  Expected<StringRef> EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
-  Expected<StringRef> UnknownVar = Cxt.getPatternVarValue(UnknownVarStr);
+  Expected<std::shared_ptr<FileCheckExpressionAST>> ExpressionAST =
+      P.parseNumericSubstitutionBlock(LocalNumVarRef, DefinedNumericVariable,
----------------
`unique_ptr`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60387/new/

https://reviews.llvm.org/D60387