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

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

jdenny 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;
----------------
jhenderson wrote:
> 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.
OK, for now I'll leave that for a separate patch.


================
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";
----------------
jhenderson wrote:
> 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.
> 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).

Nice talk.  Make sense.  You're talking about libraries there.  That means the `bool` return type for `FileCheck::ValidateCheckPrefixes` would need to change in addition to the local functions we were discussing.  `FileCheck::readCheckFile` is another that should change.  Especially because those changes would be library interface changes, I think they should go in a separate patch.

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

I've folded in `ValidateCheckPrefix`.  I think the result is easier to read, but I'm happy to try something else if people disagree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79375