[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/include/llvm/Support/FileCheck.h:100
+  /// been defined.
+  llvm::Optional<StringRef> getPatternVarValue(StringRef VarName);
+
----------------
Is there going to be a `getNumericVarValue` or similar function in the future? If not, I think this should be simply `getVarValue` or `getVariableValue`.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:102
+
+  /// Define pattern and numeric variables from definitions given on the
+  /// command line passed as a vector of VAR=VAL strings in \p CmdlineDefines.
----------------
As numeric variables don't exist at this point, your comments should probably not refer to them. You can add the reference once they start becoming relevant here.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:107
+  /// Undefine local variables (variables whose name does not start with a '$'
+  /// sign), ie remove them from GlobalVariableTable.
+  void clearLocalVars(void);
----------------
Nit: ie -> i.e.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:108
+  /// sign), ie remove them from GlobalVariableTable.
+  void clearLocalVars(void);
+};
----------------
Nit: you don't need the explicit `void` parameter.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:111
 
 struct FileCheckDiag;
 
----------------
This forward declaration should probably be above your new class, so that your class sits next to the `FileCheckPattern` class.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:138
+  /// variables at the start of any given CHECK line.
+  FileCheckPatternContext *Context;
+
----------------
Just to be clear, this is shared between all patterns?


================
Comment at: llvm/lib/Support/FileCheck.cpp:272
 ///
-/// The \p VariableTable StringMap provides the current values of filecheck
+/// The GlobalVariableTable StringMap provides the current values of FileCheck
 /// variables and is updated if this match defines new values.
----------------
I'd mention that the GlobalVariableTable is in the Context.


================
Comment at: llvm/lib/Support/FileCheck.cpp:304
       } else {
-        StringMap<StringRef>::iterator it =
-            VariableTable.find(VariableUse.first);
+        llvm::Optional<StringRef> OptValue =
+            Context->getPatternVarValue(VariableUse.first);
----------------
I don't think you need to have `Opt` in the name, as it is fairly obviously an Optional from context. Maybe better would be `VarValue`.


================
Comment at: llvm/lib/Support/FileCheck.cpp:307
         // If the variable is undefined, return an error.
-        if (it == VariableTable.end())
+        if (!OptValue.hasValue())
           return StringRef::npos;
----------------
It is more typical to do `if (!OptValue)` for Optionals.


================
Comment at: llvm/lib/Support/FileCheck.cpp:311
         // Look up the value and escape it so that we can put it into the regex.
-        Value += Regex::escape(it->second);
+        Value += Regex::escape(OptValue.getValue());
       }
----------------
More common is to do `*OptValue`.


================
Comment at: llvm/lib/Support/FileCheck.cpp:389
       } else {
-        StringMap<StringRef>::const_iterator it = VariableTable.find(Var);
+        llvm::Optional<StringRef> OptValue = Context->getPatternVarValue(Var);
 
----------------
See comments above about the name and the usage immediately below.


================
Comment at: llvm/lib/Support/FileCheck.cpp:477
 
+/// Return the value of variable \p VarName or nothing if no such variable
+/// has been defined.
----------------
nothing -> None (to be explicit).


================
Comment at: llvm/lib/Support/FileCheck.cpp:481
+FileCheckPatternContext::getPatternVarValue(StringRef VarName) {
+  auto git = GlobalVariableTable.find(VarName);
+  if (git == GlobalVariableTable.end())
----------------
"git" is a meaningless acronym to me. Perhaps simply "VarIter" or something like that would be better.


================
Comment at: llvm/lib/Support/FileCheck.cpp:761
                                     std::vector<FileCheckString> &CheckStrings) {
+  auto *PatternContext = new FileCheckPatternContext();
+  PatternContext->defineCmdlineVariables(Req.GlobalDefines);
----------------
`unique_ptr` or `shared_ptr`?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1387
+
+  if (CmdlineVariablesDefined)
+    return;
----------------
Is this necessary? Under what circumstances do we read the file twice?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1391
+
+  for (const auto &CmdlineDef : CmdlineDefines)
+    GlobalVariableTable.insert(StringRef(CmdlineDef).split('='));
----------------
Could you make this a `StringRef` here? That would simplify the code below.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1396
+/// Undefine local variables (variables whose name does not start with a '$'
+/// sign), ie remove them from GlobalVariableTable.
+void FileCheckPatternContext::clearLocalVars(void) {
----------------
Nit: ie -> i.e.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1399
+  SmallVector<StringRef, 16> LocalPatternVars, LocalNumericVars;
+  for (const auto &Var : GlobalVariableTable)
     if (Var.first()[0] != '$')
----------------
Don't use auto here. I don't know what the type is immediately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60381