Page MenuHomePhabricator

[FileCheck] Make invalid prefix diagnostics more precise
ClosedPublic

Authored by jdenny on May 4 2020, 4:27 PM.

Details

Summary

This will prove especially helpful after 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.

Diff Detail

Event Timeline

jdenny created this revision.May 4 2020, 4:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2020, 4:27 PM
jhenderson added inline comments.May 5 2020, 1:02 AM
llvm/lib/Support/FileCheck.cpp
1878

ArrayRef

1881

How about WithColor::error() instead?

1900

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?

jhenderson added inline comments.May 5 2020, 1:11 AM
llvm/lib/Support/FileCheck.cpp
1878

Also, could you name these variables more distinctly so that it's obvious which is which by name.

jdenny updated this revision to Diff 262139.May 5 2020, 9:27 AM
jdenny marked 4 inline comments as done.

Applied most reviewer suggestions.

jdenny added inline comments.May 5 2020, 9:30 AM
llvm/lib/Support/FileCheck.cpp
1881

Good idea. But should that be a separate patch that changes all FileCheck diagnostics at once?

1900

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?

thopre added inline comments.May 5 2020, 10:31 AM
llvm/lib/Support/FileCheck.cpp
1885–1887

Could you make ValidateCheckPrefix throw a DiagnosticError and print the error here?

1900

I don't think we should allow customized default directive until the need arises.

jdenny marked 2 inline comments as done.May 5 2020, 12:14 PM
jdenny added inline comments.
llvm/lib/Support/FileCheck.cpp
1885–1887

That seems ok, but what's the motivation? Is it a preferred style, or is there a technical advantage?

1900

I agree. Let's wait until there's a specific need that cannot be handled by -check-prefix[es] or FileCheckRequest::CheckPrefixes.

thopre added inline comments.May 5 2020, 2:28 PM
llvm/lib/Support/FileCheck.cpp
1885–1887

Future proofing: the function could start getting used elsewhere so having the error message there makes sense. Feel free to disagree.

jdenny marked an inline comment as done.May 5 2020, 2:47 PM
jdenny added inline comments.
llvm/lib/Support/FileCheck.cpp
1885–1887

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.

jhenderson added inline comments.Wed, May 6, 12:34 AM
llvm/lib/Support/FileCheck.cpp
1878

ArrayRef is like StringRef, immutable and basically a pointer by default, so you don't need the const & bit when using it.

1881

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.

1885–1887

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.

1900

Okay, seems fair.

jdenny updated this revision to Diff 262404.Wed, May 6, 9:46 AM
jdenny marked 11 inline comments as done.
jdenny set the repository for this revision to rG LLVM Github Monorepo.

Addressed remaining reviewer comments.

llvm/lib/Support/FileCheck.cpp
1881

OK, for now I'll leave that for a separate patch.

1885–1887

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.

jhenderson accepted this revision.Thu, May 7, 12:39 AM

LGTM.

llvm/lib/Support/FileCheck.cpp
1885–1887

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.

This revision is now accepted and ready to land.Thu, May 7, 12:39 AM
jdenny marked 2 inline comments as done.Thu, May 7, 7:04 AM

Thanks for the review!

llvm/lib/Support/FileCheck.cpp
1885–1887

some principles (don't use bools for success/fail when error messages are available) might be more widely applicable.

Agreed.

thopre accepted this revision.Thu, May 7, 7:45 AM

LGTM too

This revision was automatically updated to reflect the committed changes.
jdenny marked an inline comment as done.