[PATCH] D60387: FileCheck [7/12]: Arbitrary long numeric expressions

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Regarding what to call the `@LINE+offset` form, I think it's fine to call it "legacy" in the user-facing documentation, but it gets to be a bit much in the internals.  I haven't commented every use but you will get the idea in the inline comments.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:593
 
-The syntax of a numeric substitution is ``[[#<NUMVAR><op><offset>]]`` where:
-
-* ``<NUMVAR>`` is the name of a defined numeric variable.
-
-* ``<op>`` is an optional numeric operation to perform on the value of
-  ``<NUMVAR>``. Currently supported numeric operations are ``+`` and ``-``.
-
-* ``<offset>`` is the immediate value that constitutes the second operand of
-  the numeric operation <op>. It must be present if ``<op>`` is present,
-  absent otherwise.
-
-Spaces are accepted before, after and between any of these elements.
+The syntax of a numeric substitution is ``[[#<expr>]]`` where ``<expr>>`` is a
+numeric expression whose operands are either previously defined numeric
----------------
`<expr>>` should be `<expr>`


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:597
+``+`` and ``-``. A single numeric variable or integer literal is also accepted.
+Spaces are accepted before, after and between the operands.
 
----------------
There are a few different ways to describe expressions, I happen to prefer a recursive definition, something like this:

A numeric expression is recursively defined as:
 - a numeric operand, or
 - a numeric expression followed by an operator and a numeric operand.
A numeric operand is a previously defined numeric variable, or an integer literal. The supported operators are `+` and `-`.  Spaces are accepted before, after and between any of these elements.







================
Comment at: llvm/include/llvm/Support/FileCheck.h:43
 
-/// Class representing a numeric variable with a given value in a numeric
-/// expression. Each definition of a variable gets its own instance of this
-/// class. Variable uses share the same instance as their respective
+/// Base class representing the AST of a given numeric expression.
+class FileCheckNumExprAST {
----------------
thopre wrote:
> jhenderson wrote:
> > Are there other kinds of expressions except "numeric expressions"? Just wondering if we can simplify the terminology a little e.g. by simply referring to them as expressions and the `FileCheckNumExprAST` can become `FileCheckExpressionAST`.
> There's only numeric expression indeed but wouldn't the use of "expression" only induce the reader into thinking it only applies when there is an operator involved? Numeric expression as it stands also cover a single numeric variable in a numeric substitution. Also if changing to expression only, should I remove the "numeric" adjective in the documentation as well?
I think "expression" does not necessarily imply an operator, at least not to a programmer.
You can probably also remove some uses of "numeric" in the documentation, when you are talking about expressions (but keep it when talking about variables).


================
Comment at: llvm/include/llvm/Support/FileCheck.h:441
+  /// Parses \p Expr for a numeric substitution block. Parameter \p Legacy
+  /// indicates whether Expr should be a legacy numeric substitution block.
+  /// Sets \p NumExprAST to point to the class instance representing the
----------------
thopre wrote:
> jhenderson wrote:
> > What's a "legacy numeric substitution block"?
> It refers to the "legacy use of @LINE" mentioned in the section "FileCheck Pseudo Numeric Variables" of the documentation, namely [[@LINE]] and [[@LINE+offset]] instead of the same with a '#' sign. Would "indicates whether Expr is a legacy use of @LINE" be clearer? Or perhaps "legacy @LINE expression"? If not, any other suggestion?
I don't know about James but I would prefer that the commentary say "legacy @LINE expression" and the parameter could be named LineExpr.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:538
+                          const SourceMgr &SM) const;
+  enum AllowedOperand { LegacyVar, LegacyLiteral, Any };
+  /// Parses \p Expr for use of a numeric operand. Accepts both literal values
----------------
These could be `{ LineVar, Literal, Any }` perhaps.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:540
+  /// Parses \p Expr for use of a numeric operand. Accepts both literal values
+  /// and numeric variables, depending on the value of \p AllowedOperandFlag.
+  /// \returns the class representing that operand in the AST of the numeric
----------------
I don't see a parameter named AllowedOperandFlag.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:548
+  /// operation is given in \p LeftOp and \p Legacy indicates whether we are
+  /// parsing a legacy numeric expression. \returns the class representing the
+  /// binary operation in the AST of the numeric expression, or an error
----------------
\p LineExpr ... parsing a @LINE expression.


================
Comment at: llvm/lib/Support/FileCheck.cpp:189
+    }
+    if (AO != LegacyLiteral && AO != Any)
+      return ParseVarResult.takeError();
----------------
The previous `if` guarantees AO is LegacyVar or Any here.  So, this condition simplifies to `AO == LegacyVar` correct? (or LineVar if you rename as suggested.)


================
Comment at: llvm/lib/Support/FileCheck.cpp:191
+      return ParseVarResult.takeError();
+    consumeError(ParseVarResult.takeError());
+  }
----------------
Maybe a comment here, "Ignore the error and retry parsing as a literal."


================
Comment at: llvm/lib/Support/FileCheck.cpp:194
+
+  if (AO == LegacyLiteral || AO == Any) {
+    // Otherwise, parse it as a literal.
----------------
I think the only possibilities at this point are LegacyLiteral and Any, so this `if` is always true and should be removed.


================
Comment at: llvm/lib/Support/FileCheck.cpp:244
                                     "missing operand in numeric expression");
-  uint64_t RightOp;
-  if (Expr.consumeInteger(10, RightOp))
-    return FileCheckParseError::get(
-        SM, Expr, "invalid offset in numeric expression '" + Expr + "'");
-  Expr = Expr.ltrim(SpaceChars);
-  if (!Expr.empty())
-    return FileCheckParseError::get(
-        SM, Expr,
-        "unexpected characters at end of numeric expression '" + Expr + "'");
+  // Second operand in legacy numeric expression is a literal.
+  AllowedOperand AO = Legacy ? LegacyLiteral : Any;
----------------
... in a @LINE expression ...


================
Comment at: llvm/lib/Support/FileCheck.cpp:427
       bool IsDefinition = false;
+      bool Legacy = false;
       StringRef DefName;
----------------
IsLineExpr


================
Comment at: llvm/lib/Support/FileCheck.cpp:719
+        /*assert(UndefVarErrors.isA<FileCheckUndefVarError>() &&
+               "Unexpected error");*/
+        OS << "uses undefined variable(s):";
----------------
Delete the commented-out assertion.


================
Comment at: llvm/lib/Support/FileCheck.cpp:727
+                        [](const ErrorInfoBase &E) {
+                          assert(false && "Unexpected error");
+                        });
----------------
llvm_unreachable instead of assert(false)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387