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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:134
+  /// performing the substitutions.
+  bool isMatchTimeKnown() const;
+
----------------
I would call this `isKnownAtMatchTime()` or `isValueKnownAtMatchTime()`. To me `isMatchTimeKnown()` sounds like "is the time of the match known?" which obviously doesn't make any sense in this context but the longer name means I know what the function does without find the documentation comment.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:562
+  /// Parses \p Expr for the name of a numeric variable to be defined. \returns
+  /// an error holding a diagnostic against \p SM should defining such a
+  /// variable be invalid, or Success otherwise. In the latter case, sets
----------------
"if defining such a variable is invalid" slightly easier to parse for me.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:578
+                      FileCheckPatternContext *Context, const SourceMgr &SM);
+  /// Parses \p Expr for a binary operation at line \p LineNumber, where 0
+  /// indicates the command-line. The left operand of this binary operation is
----------------
Here and in the other functions: maybe use Optional<size_t> instead of a magic value of zero for line numbers?
I don't have a strong opinion here, I just think it might make it more obvious that the line number can also not be a line number but rather "this came from the command line".


================
Comment at: llvm/lib/Support/FileCheck.cpp:31
+
+  if (ExpressionAST == nullptr)
+    return make_error<FileCheckUndefVarError>(Name);
----------------
I think `!ExpressionAST` is the more commonly used style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388