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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1887
+    if (Prefix.empty()) {
+      errs() << "error: supplied check prefix must not be the empty string\n";
       return false;
----------------
jdenny wrote:
> jhenderson wrote:
> > How about `WithColor::error()` instead?
> Good idea.  But should that be a separate patch that changes all FileCheck diagnostics at once?
Yes, possibly. As most of these are new checks, that's why I brought it up, but at least one of them is moving, so that's fine.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1905
+
+static const char *DefaultCheckPrefixes[] = {"CHECK"};
 
----------------
jdenny wrote:
> thopre wrote:
> > jdenny wrote:
> > > jhenderson wrote:
> > > > This is only used in one place. Why not just explicitly provide it there? Alternatively, should this be defined in the top-level bit of FileCheck, so that client libraries can have different default check prefixes?
> > > > This is only used in one place. Why not just explicitly provide it there?
> > > 
> > > I changed it.  It was that way because I extracted it from D79276, which still uses it in two places.
> > > 
> > > > Alternatively, should this be defined in the top-level bit of FileCheck, so that client libraries can have different default check prefixes?
> > > 
> > > Perhaps so.  But I feel like that's an orthogonal issue and belongs in a separate patch.  What do you think?
> > I don't think we should allow customized default directive until the need arises.
> I agree. Let's wait until there's a specific need that cannot be handled by `-check-prefix[es]` or `FileCheckRequest::CheckPrefixes`.
Okay, seems fair.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1884
+static bool ValidatePrefixes(StringSet<> &UniquePrefixes,
+                             const ArrayRef<StringRef> &SuppliedPrefixes) {
+  for (StringRef Prefix : SuppliedPrefixes) {
----------------
`ArrayRef` is like `StringRef`, immutable and basically a pointer by default, so you don't need the `const &` bit when using it.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1891-1893
+      errs() << "error: supplied check prefix must start with a letter and "
+             << "contain only alphanumeric characters, hyphens, and "
+             << "underscores: '" << Prefix << "'\n";
----------------
jdenny wrote:
> thopre wrote:
> > jdenny wrote:
> > > thopre wrote:
> > > > Could you make ValidateCheckPrefix throw a DiagnosticError and print the error here?
> > > That seems ok, but what's the motivation?  Is it a preferred style, or is there a technical advantage?
> > Future proofing: the function could start getting used elsewhere so having the error message there makes sense. Feel free to disagree.
> So the goal is to put the diagnostic that describes the constraint in the function that checks the constraint.  Makes sense to me.
> 
> Do you think it's worthwhile for `ValidateCheckPrefix` to be a separate function? It's so small, I'm thinking it does more to obfuscate the logic in `ValidatePrefixes` than to improve its readability.
Passing an `Error` up the stack rather than relying on booleans seems like a good idea to me too (see my lightning talk from a couple of years ago which touched on this: https://www.youtube.com/watch?v=YSEY4pg1YB0).

I think it makes sense to fold in the function, but have no strong preference, if there's a good reason not to.


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

https://reviews.llvm.org/D79375