[PATCH] D60392: FileCheck [12/12]: Support use of var defined on same line

Wed Nov 17 01:30:01 PST 2021

jhenderson added a comment.

Sorry for ignoring this for a couple of weeks - it fell off my radar a bit, when I was swamped with deadline-realted tasks.

I recommend doing the `llvm/test/FileCheck/numeric-expressions/numeric-expressions.txt` renaming in a separate patch, as it's not really related to this patch.

I've not looked at the testing in depth yet, and I'm a little concerned that I'm very cold on the surrounding code. I think it would be useful if someone else could take a look too (@jdenny maybe?).

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:913-915
+Important note: In its current implementation, an expression using a variable
+defined on the same line can fail to match even though an input line satisfies
+it. This happens when there is more than one possible match in the line when
----------------
Where this refers to "line" should it be "CHECK" directive?

Also, I think it would be valuable to give an example.

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:917-918
+ignoring such expressions and considering only their matching format
+(ie. matching an unsigned number with [[:digit:]]+ and the first match does not
+satisfy the constraint. This is due to those expressions not being able to be
+expressed at the regular expression level and the regular expression engine not
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:382
+  if (!EvaluatedValue) {
+    consumeError(EvaluatedValue.takeError());
+    return true;
----------------
My general rule is that `consumeError` should always have a comment explaining why you can just "ignore" the error and continue. In this case, I guess you're not ignoring the error per se, but you are throwing away the context, so a comment would still be useful.

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:411-415
+  Expected<ExpressionValue> ResultValue = EvalBinop(*LeftOp, *RightOp);
+  if (!ResultValue)
+    return ResultValue;
+
+  return ResultValue;
----------------
Do you need this change? Can't you just return the result of `EvalBinop` directly?

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:449-450
+  // variable. This allows them to be substituted with the right value
+  // accordingly rather than match anything and verify later, which introduce
+  // potential false negative match.
   Expected<ExpressionValue> EvaluatedValue =
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1184
+          CapturedExpressions.push_back(CapturedExpression);
+          // TODO: reword
           // This store is done here rather than in match() to allow
----------------
TODO that needs action still?

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1307
     // now known. Use of string variables defined on the same line are handled
-    // by back-references.
+    // by back-references. For expressions using variable with empty expression
+    // defined on the same line, RegExStr is filled with a suitable wildcard
----------------
(or possibly "using variables with empty expressions")

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1311-1312
+    //
+    // Known issue: Due to the constraints for expressions using variable
+    // variable defined in earlier line not being expressed at the regular
+    // expression level, the wrong values might be matched, leading to a
----------------
(or "using variables")

Should this definitely be "line"?

Perhaps change "Known issue" to FIXME and file a bug (bug filing to happen once this patch has landed, or shortly beforehand, with the FIXME to contain a link to that bug).

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1322
+    // to be able to return all matches for a given pattern. FileCheck could
+    // then test all possibility and fail the check only if none of them
+    // satisfy all expressions. This would work both for expression using a
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1323-1324
+    // then test all possibility and fail the check only if none of them
+    // satisfy all expressions. This would work both for expression using a
+    // numeric variable defined on the same line as well as expressions with a
+    // comparison rather than equality constraint.
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1385-1386
+    // safe to do even if this iteration fails to verify all expressions
+    // because use of string variable defined on the same lined is matched by
+    // using back-reference. Only variables defined in earlier lines are
+    // matched by their value.
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1403
+    // Check matched values satisfy their corresponding expression and store
+    // them in the numeric variables being defined if any.
+    for (const auto &CapturedExpression : CapturedExpressions) {
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1424
+        // might be other possible matches in the current line but we have no
+        // way to ask for them to the regular expression engine. However this
+        // hazard can be alleviated if the user write a CHECK pattern that uses
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1425-1426
+        // way to ask for them to the regular expression engine. However this
+        // hazard can be alleviated if the user write a CHECK pattern that uses
+        // expression for all numeric values that the line to match contains.
+        Buffer = Buffer.substr(Buffer.find_first_of('\n')).substr(1);
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1431
+
+      // If this defines a numeric variables, mark the verified value as
+      // tentative and store it.
----------------

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1433
+      // tentative and store it.
+      if (DefinedNumericVariable != nullptr) {
+        DefinedNumericVariable->setValue(*Value, /*Tentative=*/true,
----------------
No need for braces for single line if.

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1438
+    }
+  } while (!AllExpressionsSatisfied && !Buffer.empty());

----------------
Any reason we can't just use a `for` loop here?

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1487

+      // Substitution succeeded and expression satisfied its constrained.
       OS << "with \"";
----------------

================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:269
+  /// Evaluates and \returns the value of this assumed non-null expression or
+  /// an error if evaluation fails. It should only be invoked on expression
+  /// with a non null AST.
----------------

================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:292-294
+  /// Tentative value of numeric variable when the constraints of the
+  /// expression where it is defined have not been verified yet.
+  Optional<ExpressionValue> TentativeValue;
----------------
Seems like `TentativeValue` and `StrTentativeValue` are unnecessary? Could you not just set `Value` and `StrValue` in this context? If you need to distinguish a tentative versus concrete definition, add a boolean member or similar that states `isTentative`.

================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:520

-  /// When matching a given pattern, this holds the pointers to the classes
-  /// representing the numeric variables defined in previous patterns. When
-  /// matching a pattern all definitions for that pattern are recorded in the
-  /// NumericVariableDefs table in the Pattern instance of that pattern.
-  StringMap<NumericVariable *> GlobalNumericVariableTable;
+  /// Holds the pointers to classes instances representing all the numeric
+  /// variables defined in patterns parsed so far and the line number where
----------------

================
Comment at: llvm/lib/FileCheck/FileCheckImpl.h:578
+                             ExpressionFormat Format, bool MatchTimeKnown,
+                             NumericVariable *DefinedNumericVariable);
   /// Makes a new string substitution and registers it for destruction when the
----------------
Nit: add new line after this method declaration.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60392