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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
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:
> 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.
Yeah, I am talking about libraries, although some principles (don't use bools for success/fail when error messages are available) might be more widely applicable. Anyway, not a change for this patch as you say.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79375