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

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

thopre marked 2 inline comments as done.
thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1386-1387
+    std::vector<std::string> &CmdlineDefines) {
+  assert(GlobalVariableTable.empty() &&
+         "Overriding defined variable with command-line variable definitions");
+  for (StringRef CmdlineDef : CmdlineDefines)
----------------
jhenderson wrote:
> I'm okay with this, but an alternative method based on a different bit of code in the DWARFDebug* parser classes, might be to explicitly call `GlobalVariableTable.clear()` at the start of this function.
> 
> No need to change unless you want to.
Command-line defines are called "GlobalDefines" in the FileCheckRequest API so my understanding is variables defined from processing the input file override the global defines rather than the reverse so no command-line definition should be processed once any variable has been defined from the input text.

Note that the API associate one FileCheckRequest to a given FileCheck class so 1 request = 1 input file and 1 directive file. There is thus no need to support the case of several input file sharing the same set of global defines. Since each input file gets its own FileCheckPatternContext, they will all get their own GlobalVariableTable which should be empty before the input text starts to be processed. This assert therefore does not get triggered in normal usage (global defined processed first) and will fire if global defines would be overriding variable defined from the input text.

I thus think this is a better approach.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1440
 
-    if (Req.EnableVarScope)
-      ClearLocalVars(VariableTable);
+    if (Req.EnableVarScope && Context != nullptr)
+      Context->clearLocalVars();
----------------
jhenderson wrote:
> Under what circumstances can Context be nullptr?
When CheckStrings is empty but I've just realized thanks to you that readCheckFile guarantees it is not empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381