[PATCH] D60386: FileCheck [6/12]: Introduce numeric variable definition

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

There's still some inconsistency in the terminology, as clearly evidenced by the name and description of the `IsNumExpr` member of `FileCheckPatternSubstitution`.  That class used to be where we'd find variables (now called pattern variables); but it has had numeric expressions crammed into it, except the comment wants to call it a substitution, and so we have "pattern" and "substitution" and "expression" all competing for clarity.

I'm uncertain what to suggest.  It may be that "pattern variable" was not a great name, and instead it should be "string variable" to contrast more naturally with "numeric variable."  And then substitutions are just substitutions, which can be either a string substitution or a numeric substitution, with the numeric substitution implicitly using a numeric expression.  Following this idea, `FileCheckPatternSubstitution` would be renamed `FileCheckSubstitution` and the references to "numeric expression" within the class don't change.  I don't know that I'd go so far as to have String and Numeric subclasses of FileCheckSubstitution, the abstraction of a Substitution might not really support that.  But the naming can surely be improved.

What do you think?  This is a discussion point, not a please-do-this comment.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:66
   FileCheckNumericVariable(StringRef Name, uint64_t Value)
       : Name(Name), Value(Value) {}
 
----------------
This leaves DefLineNumber uninitialized.  Maybe combine the two constructors into a single 3-parameter constructor? `Value` can still default to `None` if it's the last parameter.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:130
+  /// Whether this represents a numeric substitution.
   bool IsNumExpr;
 
----------------
See main comment.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:355
 public:
   explicit FileCheckPattern(Check::FileCheckType Ty,
+                            FileCheckPatternContext *Context, unsigned Line)
----------------
Multiple parameters means you don't need 'explicit'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60386