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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Starting set of comments on this one.  Haven't looked at everything yet.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:623
+Important note: In its current implementation, a numeric expression cannot use
+a numeric variable defined on the same line.
+
----------------
Which makes the above example illegal, so you should rewrite the example.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:379
+  /// Parses \p Expr for a numeric expression. \returns the class representing
+  /// the AST of numeric expression or nullptr if parsing fails in which case
+  /// errors are reported on \p SM. Sets \p DefinedNumericVariable to the
----------------
"of the numeric"
comma after "fails"


================
Comment at: llvm/include/llvm/Support/FileCheck.h:382
+  /// pointer to the class representing the variable defined to this numeric
+  /// expression if any.
+  FileCheckNumExpr *
----------------
That last sentence feels awkward... maybe:
"If the expression defines a numeric variable, sets \p DefinedNumericVariable to point to the class representing the variable."
(with maybe "or nullptr if it does not define a variable.")


================
Comment at: llvm/include/llvm/Support/FileCheck.h:397
   bool ParsePattern(StringRef PatternStr, StringRef Prefix, SourceMgr &SM,
-                    unsigned LineNumber, const FileCheckRequest &Req);
+                    const FileCheckRequest &Req);
   /// Matches the pattern string against the input buffer \p Buffer
----------------
Need to remove LineNumber from the documentation, if it's no longer a parameter.


================
Comment at: llvm/lib/Support/FileCheck.cpp:42
 llvm::Optional<uint64_t> FileCheckNumExpr::eval() const {
-  llvm::Optional<uint64_t> LeftOp = this->LeftOp->getValue();
+  assert(LeftOp != nullptr && "Evaluating an empty numeric expression");
+  llvm::Optional<uint64_t> LeftOpValue = LeftOp->getValue();
----------------
Omit the `!= nullptr` on pointer assertions.


================
Comment at: llvm/lib/Support/FileCheck.cpp:170
+  // below is for the class instance corresponding to the last definition of
+  // this variable use.
   auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
----------------
I think this comment should not have changed.


================
Comment at: llvm/lib/Support/FileCheck.cpp:289
+    return Context->makeNumExpr(add, nullptr, 0);
+  } else
+
----------------
no else after return


================
Comment at: llvm/lib/Support/FileCheck.cpp:388
+    // the double brackets and also have the definition and substitution forms.
+    // Both pattern and numeric variables must satisfy the regular expression
     // "[a-zA-Z_][0-9a-zA-Z_]*" to be valid, as this helps catch some common
----------------
variables -> variable names


================
Comment at: llvm/lib/Support/FileCheck.cpp:412
 
-      size_t VarEndIdx = MatchStr.find(":");
-      if (IsNumExpr)
-        MatchStr = MatchStr.ltrim(SpaceChars);
-      else {
+      bool IsVarDef;
+      StringRef DefName;
----------------
`= false`


================
Comment at: llvm/lib/Support/FileCheck.cpp:467
 
-      if (IsNumExpr || (!IsVarDef && IsPseudo)) {
-        NumExpr = parseNumericExpression(Name, IsPseudo, Trailer, SM);
+      // Parse numeric expression.
+      if (IsNumExpr) {
----------------
I think `NumExpr` and `DefinedNumericVariable` could be declared here, much closer to where they are used.


================
Comment at: llvm/lib/Support/FileCheck.cpp:473
         IsNumExpr = true;
+        if (DefinedNumericVariable != nullptr) {
+          IsVarDef = true;
----------------
Omit `!= nullptr`


================
Comment at: llvm/lib/Support/FileCheck.cpp:1794
       // GlobalVariableTable for that by populating it with an empty string
-      // since we would then lose the ability to detect the use of an undefined
-      // variable in Match().
+      // since we would then lose the ability to detect use of undefined
+      // variable in match().
----------------
"detect the use of an undefined"
(missing rebase maybe?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60386