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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1387
+
+  if (CmdlineVariablesDefined)
+    return;
----------------
thopre wrote:
> 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?
I'd actually look at it a different way. Clear the variables list and only add the new values. That way this class is more flexible. The problem with a static local is that it is just global state with a local visibility. This would make it impossible to, for example, write unit tests that use this code. Speaking of which, how practical would adding unit tests be for the new code?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1416
 
-  /// VariableTable - This holds all the current filecheck variables.
-  StringMap<StringRef> VariableTable;
-
-  for (const auto& Def : Req.GlobalDefines)
-    VariableTable.insert(StringRef(Def).split('='));
-
+  std::shared_ptr<FileCheckPatternContext> Context =
+      CheckStrings.empty() ? nullptr : CheckStrings[0].Pat.getContext();
----------------
In this case you can just use the raw pointer referenced by the shared_ptr, as you don't need to own the pointer here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381