Page MenuHomePhabricator

Allow matching "any file" in `VerifyDiagnosticConsumer`.
ClosedPublic

Authored by arames on Jan 2 2020, 12:54 PM.

Details

Summary

This allows specifying * for the filename to match any file. For example:

// expected-note@*:* {{Match this note in any file at any line.}}

Diff Detail

Event Timeline

arames created this revision.Jan 2 2020, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2020, 12:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arames added a comment.Jan 2 2020, 1:00 PM

This is for example useful to add a catch-all clause like // expected-note-re@*:* 1+ {{candidate function {{.+}}}}

jkorous added inline comments.
clang/test/Frontend/verify-any-file.c
2

I feel that we should test the output with FileCheck to make it more robust.
Seems like other tests for -verify do that. For example here:
https://github.com/llvm/llvm-project/blob/master/clang/test/Frontend/verify-fatal.c

7

Can we add some scenarios for expected diagnostics not produced?

arames marked 3 inline comments as done.Jan 23 2020, 10:32 AM
arames added inline comments.
clang/test/Frontend/verify-any-file.c
5

While testing, I realized this cannot be handled reliably, so will require the line number to be * when the filename is *.

I was using the current file and specified line for the SourceLocation in this case. But this fails if the specified line number is greater than the number of lines in the current file.

arames updated this revision to Diff 239972.Jan 23 2020, 12:07 PM

Address review comments.

We should either simplify the implementation to reflect that we don't support e. g. *:42 (seems preferable to me) or have the codepaths that are currently not accessible through -fverify tested by other means.

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
207

Should we rename it to MatchAnyFileAndLine?

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
300

Should we make it clear from the interface that MatchAnyFile => MatchAnyLine?

arames marked 2 inline comments as done.Feb 26 2020, 5:54 PM

We should either simplify the implementation to reflect that we don't support e. g. *:42 (seems preferable to me) or have the codepaths that are currently not accessible through -fverify tested by other means.

That makes sense. I have updated the naming and added a comment to reflect that. PTAL.
As a note and future reference, the issue was not on the side of the verification; I tested combinations of MatchAnyFile and MatchAnyLine, but the fact that we cannot always create a source location with the appropriate line number when we do not know what file we are dealing with.

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
300

The naming should now make it clear. Additionally, I have added a comment for the implication in Directive.

arames updated this revision to Diff 247087.Feb 27 2020, 1:11 PM

Rename and clarify.

arames updated this revision to Diff 250247.Mar 13 2020, 10:01 AM

Apply clang-format.

arames edited the summary of this revision. (Show Details)May 14 2020, 11:21 AM
arames updated this revision to Diff 264048.May 14 2020, 12:00 PM
arames edited the summary of this revision. (Show Details)

Rebase on top of tree.

jkorous accepted this revision.May 14 2020, 12:44 PM

LGTM! Thanks for the work!

This revision is now accepted and ready to land.May 14 2020, 12:44 PM
This revision was automatically updated to reflect the committed changes.