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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Have you looked at unit-testing the new code? It might be nice if you can.



================
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)
----------------
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.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1440
 
-    if (Req.EnableVarScope)
-      ClearLocalVars(VariableTable);
+    if (Req.EnableVarScope && Context != nullptr)
+      Context->clearLocalVars();
----------------
Under what circumstances can Context be nullptr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381