[PATCH] D79375: [FileCheck] Make invalid prefix diagnostics more precise

[Joel E. Denny via Phabricator via llvm-commits]

jdenny created this revision.
jdenny added reviewers: probinson, thopre, hfinkel, jhenderson, jroelofs, jdoerfert.
Herald added subscribers: hiraditya, arichardson.
Herald added a project: LLVM.
jdenny added a child revision: D79276: [FileCheck] Support comment directives.

This will prove especially helpful after D79276 <https://reviews.llvm.org/D79276>, which introduces
comment prefixes.  Specifically, identifying whether there's a
uniqueness violation will be helpful as prefixes will be required to
be unique across both check prefixes and comment prefixes.

Also, remove a related comment about `cl::list` that no longer seems
relevant now that FileCheck is also a library.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79375

Files:
  llvm/lib/Support/FileCheck.cpp
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp


Index: llvm/utils/FileCheck/FileCheck.cpp
===================================================================
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -601,12 +601,8 @@
     Req.Verbose = true;
 
   FileCheck FC(Req);
-  if (!FC.ValidateCheckPrefixes()) {
-    errs() << "Supplied check-prefix is invalid! Prefixes must be unique and "
-              "start with a letter and contain only alphanumeric characters, "
-              "hyphens and underscores\n";
+  if (!FC.ValidateCheckPrefixes())
     return 2;
-  }
 
   Regex PrefixRE = FC.buildCheckPrefixRegex();
   std::string REError;
Index: llvm/test/FileCheck/validate-check-prefix.txt
===================================================================
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -1,10 +1,13 @@
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
 // RUN: FileCheck -check-prefix=A1a-B_c -input-file %s %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=DUPLICATE_PREFIX %s
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=VALID -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix= -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix= -input-file %s %s 2>&1 | FileCheck -check-prefix=EMPTY_PREFIX %s
 foobar
 ; A1a-B_c: foobar
 
-; BAD_PREFIX: Supplied check-prefix is invalid! Prefixes must be
-  unique and start with a letter and contain only alphanumeric characters, hyphens and underscores
+; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
+
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+
+; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/lib/Support/FileCheck.cpp
===================================================================
--- llvm/lib/Support/FileCheck.cpp
+++ llvm/lib/Support/FileCheck.cpp
@@ -1880,29 +1880,41 @@
   return Validator.match(CheckPrefix);
 }
 
-bool FileCheck::ValidateCheckPrefixes() {
-  StringSet<> PrefixSet;
-
-  for (StringRef Prefix : Req.CheckPrefixes) {
-    // Reject empty prefixes.
-    if (Prefix.empty())
+static bool ValidatePrefixes(StringSet<> &PrefixSet,
+                             const std::vector<StringRef> &Prefixes) {
+  for (StringRef Prefix : Prefixes) {
+    if (Prefix.empty()) {
+      errs() << "error: supplied check prefix must not be the empty string\n";
       return false;
-
-    if (!PrefixSet.insert(Prefix).second)
+    }
+    if (!ValidateCheckPrefix(Prefix)) {
+      errs() << "error: supplied check prefix must start with a letter and "
+             << "contain only alphanumeric characters, hyphens, and "
+             << "underscores: '" << Prefix << "'\n";
       return false;
-
-    if (!ValidateCheckPrefix(Prefix))
+    }
+    if (!PrefixSet.insert(Prefix).second) {
+      errs() << "error: supplied check prefix must be unique among check "
+             << "prefixes: '" << Prefix << "'\n";
       return false;
+    }
   }
+  return true;
+}
+
+static const char *DefaultCheckPrefixes[] = {"CHECK"};
 
+bool FileCheck::ValidateCheckPrefixes() {
+  StringSet<> PrefixSet;
+  if (!ValidatePrefixes(PrefixSet, Req.CheckPrefixes))
+    return false;
   return true;
 }
 
 Regex FileCheck::buildCheckPrefixRegex() {
-  // I don't think there's a way to specify an initial value for cl::list,
-  // so if nothing was specified, add the default
   if (Req.CheckPrefixes.empty()) {
-    Req.CheckPrefixes.push_back("CHECK");
+    for (const char *Prefix : DefaultCheckPrefixes)
+      Req.CheckPrefixes.push_back(Prefix);
     Req.IsDefaultCheckPrefix = true;
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79375.261956.patch
Type: text/x-patch
Size: 4188 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200504/19cbb950/attachment-0001.bin>