[PATCH] D22403: FileCheck Enhancement - pattern templates.

[Vedant Kumar via llvm-commits]

vsk added a comment.

More mostly surface-level comments. I think Adrian and I both have concerns over the syntax, so I'll wait a bit before diving into the code.


================
Comment at: docs/CommandGuide/FileCheck.rst:522
@@ +521,3 @@
+regName is parameter instead of which will be placed another string at the moment of matching. 
+There can be mane parameters and parameters can be used some times in different parts of template.
+To use such templates you need to set values for each parameter.
----------------
mane -> many

================
Comment at: docs/CommandGuide/FileCheck.rst:529
@@ +528,3 @@
+
+    // CHECK :  INC  \#( reg_hex_num)\:(regName)\=(reg)
+
----------------
Is the extra space in `\#( reg_hex_num)` necessary? I don't want to be too picky / focused on minutiae, but I think this could legitimately be a point of confusion.

================
Comment at: docs/CommandGuide/FileCheck.rst:529
@@ +528,3 @@
+
+    // CHECK :  INC  \#( reg_hex_num)\:(regName)\=(reg)
+
----------------
vsk wrote:
> Is the extra space in `\#( reg_hex_num)` necessary? I don't want to be too picky / focused on minutiae, but I think this could legitimately be a point of confusion.
Have you considered changing the syntax for parameter substitution? IMO `\:(regName)\=(reg)` has two problems: (1) it uses two separate syntactic constructs to do one thing, and (2) it overloads `\:`, making its meaning context-dependent. I suggest something like this: `\=(regName:reg)`.

================
Comment at: docs/CommandGuide/FileCheck.rst:540
@@ +539,3 @@
+Syntax:
+\#(template_name) - use of template 'template_name'
+\:(Variable_name) - template variable with name 'variable_name'
----------------
Specify whether or not this can occur in a `CHECK-PATTERN` line.

================
Comment at: test/FileCheck/check-pattern-prefixes.txt:3
@@ +2,3 @@
+// RUN: FileCheck -input-file %s %s -check-prefix=BAR
+
+// FOO-PATTERN thing: foo-thing
----------------
Check for conflicting parameter definitions:

`// RUN: not FileCheck -input-file %s %s -check-prefixes=FOO,BAR`

================
Comment at: utils/FileCheck/FileCheck.cpp:137
@@ +136,3 @@
+  /// getRegExStr - Return regex pattern.
+  std::string getRegExStr() const { return RegExStr; }
+
----------------
Should return a StringRef. If the caller needs a mutable copy, it can then do: `getRegExStr().str()`.

================
Comment at: utils/FileCheck/FileCheck.cpp:140
@@ +139,3 @@
+  /// getRegExStr - Return fixed string.
+  std::string getFixedStr() const {return FixedStr.str();}
+
----------------
Ditto, return a StringRef (avoids a copy).

================
Comment at: utils/FileCheck/FileCheck.cpp:194
@@ +193,3 @@
+  /// \return true if there is error in regular expression
+  bool ReplaceTemplates(StringRef RS, SourceMgr &SM, std::string &ResultRegex);
+};
----------------
Since there is only one in-out parameter here, I think the API would be clearer as:

`Expected<std::string> ReplaceTemplates(StringRef Str, SourceMgr &SM)` (see llvm/Support/Error.h)

================
Comment at: utils/FileCheck/FileCheck.cpp:203
@@ +202,3 @@
+  /// Template parameters.
+  std::map<size_t, StringRef> Parameters;
+
----------------
Use `llvm/ADT/IndexedMap.h`.

================
Comment at: utils/FileCheck/FileCheck.cpp:206
@@ +205,3 @@
+public:
+  PatternTemplate(std::string Name,
+                  Check::CheckType Ty = Check::CheckType::CheckNone)
----------------
`StringRef Name`

================
Comment at: utils/FileCheck/FileCheck.cpp:220
@@ +219,3 @@
+  /// Get template name.
+  /// \return template name.
+  const std::string &GetTemplateName() const;
----------------
Could you drop the `\return` doxygen line here? I don't think it adds very much.

================
Comment at: utils/FileCheck/FileCheck.cpp:246
@@ +245,3 @@
+  size_t DiffInLoc = 0;
+  StringRef PatternString = StringRef(TemplatePattern.getRegExStr());
+
----------------
Can drop `StringReg(...)` if getRegExStr() returns a StringRef.

================
Comment at: utils/FileCheck/FileCheck.cpp:307
@@ +306,3 @@
+  /// Described templates
+  StringMap<PatternTemplate *> Templates;
+  /// Last added template (for generating error)
----------------
Use `std::unique_ptr<PatternTemplate>`.

================
Comment at: utils/FileCheck/FileCheck.cpp:315
@@ +314,3 @@
+  static TemplatesCollection &Instance() {
+    static TemplatesCollection singleton;
+    return singleton;
----------------
Can you find a way to instantiate a TemplatesCollection earlier, and to then pass it by reference into methods/classes which need it? I don't think a singleton approach is much better than having a global.

================
Comment at: utils/FileCheck/FileCheck.cpp:321
@@ +320,3 @@
+  /// Get template by its name.
+  PatternTemplate *GetTemplate(const StringRef &TemplateName);
+  /// Get last added template
----------------
StringRef's are by nature already references. There's no need to pass them by-reference, just copy them.

================
Comment at: utils/FileCheck/FileCheck.cpp:1015
@@ +1014,3 @@
+    if (!TemplatesCollection::Instance().AddTemplate(
+            new PatternTemplate(Matches[1]))) {
+      SM.PrintMessage(
----------------
Use `llvm::make_unique<PatternTemplate>`.


https://reviews.llvm.org/D22403