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

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

thopre added inline comments.


================
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(
----------------
jhenderson wrote:
> 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?
It is a maintenance nightmare too. However it is useful in the header to document the API and in the code as a comment. It seems to be also the case for existing functions. It does seem that in the rest of LLVM the comments are in the header usually, with occasional comment in the source as well. I have thus followed your suggestion and removed the comment in the cpp file, except when already present.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:44
+  EXPECT_FALSE(LocalVar);
+  EXPECT_TRUE(GlobalVar);
+}
----------------
jhenderson wrote:
> I would add one last check to this test: re-add a local variable and show that you can fetch it again.
The API does not provide a function to add a variable so I'm not sure how I would do that. FileCheckPattern is friend of FileCheckPatternContext so can add it directly so as to avoid creating a any user of the class to be able to add variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381