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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
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 {
----------------
jhenderson wrote:
> 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`.
There's only numeric expression indeed but wouldn't the use of "expression" only induce the reader into thinking it only applies when there is an operator involved? Numeric expression as it stands also cover a single numeric variable in a numeric substitution. Also if changing to expression only, should I remove the "numeric" adjective in the documentation as well?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:66-67
+public:
+  /// Constructor for an unsigned literal.
+  FileCheckNumExprLiteral(uint64_t Val) : Value(Val) {}
+
----------------
jhenderson wrote:
> Are there plans for signed literals later? If not, then I'd get rid of "unsigned" from the comment.
There are but the comment can be changed when they are introduced. Fixed.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:130
   /// Left operand.
-  FileCheckNumericVariable *LeftOp;
+  std::shared_ptr<FileCheckNumExprAST> LeftOp;
 
----------------
jhenderson wrote:
> 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.
Some AST nodes are referenced from a single place only (eg. literals and binary operations) while numeric variables can be shared among several numeric substitutions, eg. "add r[[#REG]], r[[#REG+1]], 42" would have 2 pointers to the class instance for REG. In the first case std::unique_ptr could be used, while in the latter case std::shared_ptr would make sense.

The problem comes from AST nodes being referred via FileCheckAST * pointers (since each operand can be either of the possible nodes) and thus I need to decide on a given pointer type for all nodes. I could go with regular pointers and keep an array of pointer in FileCheckPatternContext to all nodes created for them to be freed all at once when matching is done. Wouldn't change much over what is happening now since substitutions are only deleted once all pattern have been matched and substitutions points to the AST indirectly. Does that sound good?


================
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
----------------
jhenderson wrote:
> What's a "legacy numeric substitution block"?
It refers to the "legacy use of @LINE" mentioned in the section "FileCheck Pseudo Numeric Variables" of the documentation, namely [[@LINE]] and [[@LINE+offset]] instead of the same with a '#' sign. Would "indicates whether Expr is a legacy use of @LINE" be clearer? Or perhaps "legacy @LINE expression"? If not, any other suggestion?


================
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
----------------
jhenderson wrote:
> I don't like the terminology "Legacy", as it doesn't convey any useful meaning, unless defined somewhere clearly.
See above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387