[PATCH] D60381: FileCheck [1/12]: Move variable table in new object

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

thopre marked 15 inline comments as done.
thopre added a comment.

Thanks for such a quick review James! I addressed all your comments, except perhaps the one about guarding against DefineCmdlineVariables being called twice. Let me know if my fix seems sensible to you.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:100
+  /// been defined.
+  llvm::Optional<StringRef> getPatternVarValue(StringRef VarName);
+
----------------
jhenderson wrote:
> Is there going to be a `getNumericVarValue` or similar function in the future? If not, I think this should be simply `getVarValue` or `getVariableValue`.
There is no getNumericVarValue in the future but this function is only for Pattern variable. Numeric variable use are represented by a pointer to the structure that holds the value so there is no need for a function. That said the term of pattern variable is introduced later so I could rename it later.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:138
+  /// variables at the start of any given CHECK line.
+  FileCheckPatternContext *Context;
+
----------------
jhenderson wrote:
> Just to be clear, this is shared between all patterns?
Yes, updated the comment to make it clear.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1387
+
+  if (CmdlineVariablesDefined)
+    return;
----------------
jhenderson wrote:
> Is this necessary? Under what circumstances do we read the file twice?
I wanted to avoid this function being called twice by mistake and thus overriding variable values read from the input file by the value on the command-line. An assert is fine but it'll still need that static variable. Is that ok?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381