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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Grammar nits and one longer point, which is:
I understand where the "parenthesis group" term came from, but I think it's not appropriate here.  While a CHECK line is implicitly a regex, and variables are a kind of back-reference, the syntax for variable references (use or def) is not parentheses and the content is not a "group" in any real sense, and so "parenthesis group" is an un-obvious and confusing term.
Because FileCheck uses square brackets, I propose that the least disruptive change at this point would be to call them "bracket expressions." The "bracket" part is obvious, and "expression" because (a) definitions will have a regex, and (b) numeric variable references are implicitly expressions.  (Okay, still a bit of a stretch, but less so that "parenthesis group" IMO.)
WDYT?

Also let me apologize in advance for the review sparsity lately, and going forward.  I was at a conference all last week, and I will be on holiday with limited internet through the end of next week.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:411
+  /// error if \p Context already holds a string variable with the same name.
+  /// \returns whether parsing fails in which case errors are reported on
+  /// \p SM. Otherwise, sets \p Name to the name of the parsed numeric
----------------
Comma after "fails"


================
Comment at: llvm/include/llvm/Support/FileCheck.h:480
+  /// the class instance representing that variable if successful, or nullptr
+  /// otherwise, which case errors are reported on \p SM.
+  FileCheckNumericVariable *parseNumericVariableUse(StringRef &Expr,
----------------
in which case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60386