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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Basically looks good to me. Just a few small comments remaining.



================
Comment at: llvm/lib/Support/FileCheck.cpp:1381-1382
 
-// Remove local variables from \p VariableTable. Global variables
-// (start with '$') are preserved.
-static void ClearLocalVars(StringMap<StringRef> &VariableTable) {
-  SmallVector<StringRef, 16> LocalVars;
-  for (const auto &Var : VariableTable)
+/// Define variables from definitions given on the command line passed as a
+/// vector of VAR=VAL strings in \p CmdlineDefines.
+void FileCheckPatternContext::defineCmdlineVariables(
----------------
I might be wrong, but I'm not sure you need (identical) doxygen comments in both source and header, so I think this and similar ones can be removed?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1393
+/// sign), i.e. remove them from GlobalVariableTable.
+void FileCheckPatternContext::clearLocalVars(void) {
+  SmallVector<StringRef, 16> LocalPatternVars, LocalNumericVars;
----------------
Nit: no need for the `void` parameter.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:44
+  EXPECT_FALSE(LocalVar);
+  EXPECT_TRUE(GlobalVar);
+}
----------------
I would add one last check to this test: re-add a local variable and show that you can fetch it again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381