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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've reviewed up to the tests, and got lots of comments for you.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:594-595
+The syntax of a numeric substitution is ``[[#<expr>]]`` where ``<expr>>`` is a
+numeric expression whose operands are either numeric variables previously
+defined or integer literals.  Currently supported numeric operations are ``+``
+and ``-``. A single numeric variable or integer literal is also accepted.
----------------
numeric variables previously defined -> previously defined numeric variables


================
Comment at: llvm/include/llvm/Support/FileCheck.h:43
 
-/// Class representing a numeric variable with a given value in a numeric
-/// expression. Each definition of a variable gets its own instance of this
-/// class. Variable uses share the same instance as their respective
+/// Base class representing the AST of a given numeric expression.
+class FileCheckNumExprAST {
----------------
Are there other kinds of expressions except "numeric expressions"? Just wondering if we can simplify the terminology a little e.g. by simply referring to them as expressions and the `FileCheckNumExprAST` can become `FileCheckExpressionAST`.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:66-67
+public:
+  /// Constructor for an unsigned literal.
+  FileCheckNumExprLiteral(uint64_t Val) : Value(Val) {}
+
----------------
Are there plans for signed literals later? If not, then I'd get rid of "unsigned" from the comment.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:69-70
+
+  /// Evaluates and returns the value of the expression represented by this
+  /// AST. Therefore, \returns the literal's value.
+  Optional<uint64_t> eval() const { return Value; }
----------------
You could probably simplify this down to `\returns the literal's value.`


================
Comment at: llvm/include/llvm/Support/FileCheck.h:103-104
 
-  /// \returns value of this numeric variable.
-  Optional<uint64_t> getValue() const { return Value; }
+  /// Evaluates and returns the value of the expression represented by this
+  /// AST. Therefore, \returns this variable's value.
+  Optional<uint64_t> eval() const { return Value; }
----------------
Similar comment to above. No need to explain that its evaluating the expression for a single variable. Just keep the "returns this variable's value" part.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:130
   /// Left operand.
-  FileCheckNumericVariable *LeftOp;
+  std::shared_ptr<FileCheckNumExprAST> LeftOp;
 
----------------
Do these need to be shared_ptrs? Do multiple places need ownership, or is it sufficient that only place has it? Same question applies for all uses of shared_ptr further down.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:152
+  /// binary operation.
+  void appendUndefVarNames(std::vector<StringRef> &UndefVarNames) const;
 };
----------------
Could this use a `MutableArrayRef`?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:193-194
 
-  /// \returns the name of the variable used in this substitution if undefined,
-  /// or an empty string otherwise.
-  virtual StringRef getUndefVarName() const = 0;
+  /// Records in \p UndefVarNames the name of the variables used in this
+  /// substitution, if undefined.
+  virtual void
----------------
How about "the name of the undefined variables used in this substitution"?

Also use "Appends" instead of "Records" to show that it doesn't override the old ones.

Same comments apply to the other examples of `getUndefVarNames`.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:209-210
 
-  /// \returns the name of the string variable used in this substitution if
-  /// undefined, or an empty string otherwise.
-  StringRef getUndefVarName() const override;
+  /// Records in \p UndefVarNames the name of the string variables used in this
+  /// substitution, if undefined.
+  void getUndefVarNames(std::vector<StringRef> &UndefVarNames) const override;
----------------
What if some string variables are defined and others aren't? The comment as it stands suggests that all are stored. If that's not intended, I'd rephrase it as "the name of the undefined string variables used in this substitution"


================
Comment at: llvm/include/llvm/Support/FileCheck.h:441
+  /// Parses \p Expr for a numeric substitution block. Parameter \p Legacy
+  /// indicates whether Expr should be a legacy numeric substitution block.
+  /// Sets \p NumExprAST to point to the class instance representing the
----------------
What's a "legacy numeric substitution block"?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:446
+  /// variable defined in this numeric substitution block, or nullptr if this
+  /// block does not defined any variable. \returns whether parsing fails, in
+  /// which case errors are reported on \p SM.
----------------
defined -> define


================
Comment at: llvm/include/llvm/Support/FileCheck.h:509
+  parseNumericVariableUse(StringRef &Expr, const SourceMgr &SM) const;
+  enum AllowedOperand { LegacyVar, LegacyLiteral, Any };
+  /// Parses \p Expr for use of a numeric operand. Accepts both literal values
----------------
I don't like the terminology "Legacy", as it doesn't convey any useful meaning, unless defined somewhere clearly.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:516
+  std::shared_ptr<FileCheckNumExprAST>
+  parseNumericOperand(StringRef &Expr, enum AllowedOperand AO,
+                      const SourceMgr &SM) const;
----------------
I don't think you need `enum` here. Same applies throughout the code.


================
Comment at: llvm/lib/Support/FileCheck.cpp:81
+    std::vector<StringRef> &UndefVarNames) const {
+  UndefVarNames.clear();
   // Although a use of an undefined numeric variable is detected at parse
----------------
Are you sure you want to clear this? That seems unnecessary, and confusing if you are passing in a reference. If you want an empty vector, I'd just expect the vector to be returned.


================
Comment at: llvm/lib/Support/FileCheck.cpp:208
+                                      const SourceMgr &SM) const {
+
+  // Try to parse as a numeric variable use.
----------------
Get rid of this blank line.


================
Comment at: llvm/lib/Support/FileCheck.cpp:222
+  uint64_t LiteralValue;
+  if (Expr.consumeInteger(10, LiteralValue))
+    return nullptr;
----------------
Magic number?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387