[PATCH] D60388: FileCheck [8/12]: Define numeric var from expr

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:632
+variables and checking a numeric expression is thus
+``[[#%<fmtspec>,<NUMVAR>: <constraint> <expr>]]`` with each element as
+described previously.
----------------
If I'm not mistaken, the %<fmtspec> bit isn't implemented in this patch?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:113
+  /// defined before input is parsed if DefLineNumber is None. If not null,
+  /// the value of the variable is given by evaluating \p ExpressionAST.
   FileCheckNumericVariable(StringRef Name,
----------------
Add to this comment what the value of the variable is if ExpressionAST is null.


================
Comment at: llvm/lib/Support/FileCheck.cpp:37
+  if (ExpressionAST != nullptr) {
+    Expected<uint64_t> EvaluatedValue = ExpressionAST->eval();
+    assert(EvaluatedValue &&
----------------
This is unchecked in builds without assertions.


================
Comment at: llvm/lib/Support/FileCheck.cpp:39
+    assert(EvaluatedValue &&
+           "Cannot evaluate expression to test against value set");
+    assert(*EvaluatedValue == NewValue && "Value set different from evaluated");
----------------
I can't parse this string here. I can't even really figure out what this means to suggest an alternative.


================
Comment at: llvm/lib/Support/FileCheck.cpp:45-47
+  // substitution that owns the pointer. While this is not currently necessary
+  // because all substitutions remain live until they have all been processed,
+  // this might avoid debugging headache in the future.
----------------
I'm not sure this second sentence is necessary?


================
Comment at: llvm/lib/Support/FileCheck.cpp:546
+        else
+          MatchRegexp = StringRef("[0-9]+");
       }
----------------
You don't need the explicit StringRef(...) here. Just do `MatchRegexp = "[0-9]+";`


================
Comment at: llvm/lib/Support/FileCheck.cpp:568
+          // string and numeric variables in parseNumericVariableUse() and
+          // DefineCmdlineVariables() when the latter is created later than the
+          // former. We cannot reuse GlobalVariableTable for this by populating
----------------
DefineCmdlineVariables doesn't conform to style guide. I haven't bothered looking it up, but I imagine this comment is a bit stale, or the function needs renaming?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1814
+  StringRef Suffix1 = " (parsed as: [[";
+  StringRef Suffix3 = "]])";
+  SmallVector<std::pair<size_t, size_t>, 4> CmdlineDefsIndices;
----------------
Suffix3? What happened to Suffix2?

Honestly, these names don't make a huge amount of sense to me.

Also, do we need all of them? As far as I can see, Suffix1 and Suffix3 are only used once each, so should just be inlined.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1817
+  for (StringRef CmdlineDef : CmdlineDefines) {
+    StringRef DefNo = Twine(++I).str();
+    size_t EqIdx = CmdlineDef.find('=');
----------------
DefNo -> DefIndex

("No" here is not sufficiently clear as to its context, IMO).


================
Comment at: llvm/lib/Support/FileCheck.cpp:1827
+      // as in the input file to be able to reuse
+      // ParseNumericSubstitutionBlock.
+      CmdlineDefsDiag +=
----------------
Should this be parseNumericSubstitutionBlock?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1854
+                                                     CmdlineDefIndices.second);
+    // Error already handled in previous loop, skip this.
+    if (CmdlineDef.empty()) {
----------------
This comment sounds like a lie? You aren't skipping the error here...


================
Comment at: llvm/lib/Support/FileCheck.cpp:1865-1866
     if (CmdlineDef[0] == '#') {
-      StringRef CmdlineName = CmdlineDef.substr(1, EqIdx - 1);
-      Expected<FileCheckNumericVariable *> ParseResult =
-          FileCheckPattern::parseNumericVariableDefinition(CmdlineName, this,
-                                                           None, SM);
-      if (!ParseResult) {
-        Errs = joinErrors(std::move(Errs), ParseResult.takeError());
+      // Now parse to both check syntax is correct and create the necessary
+      // class instance.
+      StringRef CmdlineDefExpr = CmdlineDef.substr(1);
----------------
"Parse the definition both to check that the syntax is correct and to create ..."


================
Comment at: llvm/lib/Support/FileCheck.cpp:1878
+          std::move(*ExpressionASTResult);
+      Expected<uint64_t> Value = ExpressionAST->eval();
+      if (!Value) {
----------------
Why is the expression being evaluated here? This probably needs a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388