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

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

thopre marked 3 inline comments as done.
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(
----------------
probinson wrote:
> thopre wrote:
> > 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.
> FTR, I'm quite sure doxygen comments go in the header if there is one.  FileCheck was not tidied up when it moved to be a library, I think; would be worth a separate cleanup patch after the functional stuff is done, to get that all sorted.
Which is what I did in this patch and am similarly removing comments from the cpp files in later patches.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:44
+  EXPECT_FALSE(LocalVar);
+  EXPECT_TRUE(GlobalVar);
+}
----------------
jhenderson wrote:
> jhenderson wrote:
> > thopre wrote:
> > > 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.
> > Okay, just waiting for this point to be addressed before I LGTM this.
> Oh, right, yes of course. I forgot you decided to go the assert route in defineCmdlineVariables. Perhaps in another test you could run defineCmdlineVariables with just locals, clear them, then run it again?
See my answer above. Do you want me to create a function to set a variable and make it public?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381