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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
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
----------------
probinson wrote:
> 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.
I went for LegacyLineExpr then since when false @LINE is also allowed and when true it limits what can be done with @LINE.


================
Comment at: llvm/lib/Support/FileCheck.cpp:194
+
+  if (AO == LegacyLiteral || AO == Any) {
+    // Otherwise, parse it as a literal.
----------------
probinson wrote:
> I think the only possibilities at this point are LegacyLiteral and Any, so this `if` is always true and should be removed.
I've addressed this comment and the other one about AO above but I wanted to say that although I realize that LLVM code style aims at reducing clutter to make the code more readable, I feel in many case (eg. here and some of the suppression of else after return) it makes the code less explicit and more fragile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387