This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] fix check_clang_tidy to forbid mixing of CHECK-NOTES and CHECK-MESSAGES
ClosedPublic

Authored by JonasToth on Aug 28 2018, 2:00 PM.

Details

Summary

The check_clang_tidy.py script would allow mixing of CHECK-NOTES and CHECK-MESSAGES but running FileCheck for that would implicitly fail, because CHECK-NOTES bails out if there is a warning.

That means a clang-tidy test can not mix these constructs to check warnings with CHECK-MESSAGES and notes with CHECK-NOTES. The script gives now a clear error if that happens.

Event Timeline

JonasToth created this revision.Aug 28 2018, 2:00 PM

You would still have to duplicate the check-lines for error: though.

As it was discussed in https://reviews.llvm.org/D36892 i would think the current behavior is correct:
the CHECK-MESSAGES vs CHECK-NOTES denotes the severity of the output msg that is ok to be ignored.
If you are ok with ignoring notes, just use the CHECK-MESSAGES prefix.
If you want the notes to be checked, use CHECK-NOTES prefix.
Either/or. But don't mix them.

It's not a good idea to mix them, e.g. because the tool then has to run FileCheck twice..

@lebedev.ri lets do it in the the other patch, to not split discussions. But what do you mean by You would still have to duplicate the check-lines for error: though.?

@lebedev.ri lets do it in the the other patch, to not split discussions.

Let's do it here instead, since that differential requires some changes to this script.

But what do you mean by You would still have to duplicate the check-lines for error: though.?

In it's current state, CHECK-MESSAGES implies -implicit-check-not={{warning|error}},
and CHECK-NOTES will imply -implicit-check-not={{note|error}}.
I.e. they both imply -implicit-check-not=error.
So if the check produces any error: output, and you want to use both the CHECK-NOTES and CHECK-MESSAGES,
you need to write the check-line for every error: with both the CHECK-NOTES and CHECK-MESSAGES prefixes.
This seems sub-optimal.

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

So what was the issue? Do you get the same results if you undo the D51381 and s/CHECK-MESSAGES/CHECK-NOTES/?

You are right that replacing all of it with CHECK-NOTES works as well.
FileCheck is run twice if you have FIXMES as well. Having another run for the notes is consistent with how it worked before.
If we go for the catch-all-with-one approach it might be a good idea to ensure that only one of CHECK-MESSAGES or CHECK-NOTES is present in the file and adjust the check_clang_tidy.py script a little.

I don't have a strong preference, but if that works i would prefer to go that route, since that is what the initial intended semantics of the CHECK-NOTES.
If that works for you?

Yes, you are write.

I would ensure in the script that CHECK-MESSAGES XOR CHECK-NOTES is
used, to avoid the mistake i made. I can do it in this diff as well.

Am 29.08.2018 um 20:29 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

In https://reviews.llvm.org/D51381#1217047, @JonasToth wrote:

@lebedev.ri lets do it in the the other patch, to not split discussions.

Let's do it here instead, since that differential requires some changes to this script.

In https://reviews.llvm.org/D51381#1217047, @JonasToth wrote:

But what do you mean by You would still have to duplicate the check-lines for error: though.?

In it's current state, CHECK-MESSAGES implies -implicit-check-not={{warning|error}},
and CHECK-NOTES will imply -implicit-check-not={{note|error}}.
I.e. they both imply -implicit-check-not=error.
So if the check produces any error: output, and you want to use both the CHECK-NOTES and CHECK-MESSAGES,
you need to write the check-line for every error: with both the CHECK-NOTES and CHECK-MESSAGES prefixes.
This seems sub-optimal.

In https://reviews.llvm.org/D48714#1217044, @JonasToth wrote:

In https://reviews.llvm.org/D48714#1216989, @lebedev.ri wrote:

In https://reviews.llvm.org/D48714#1216537, @JonasToth wrote:

I had to revert the CHECK-NOTES change that @lebedev.ri introduced with his revision. It fails the test, i think there is an inconsistency or so in the check-clang-tidy script. I will try to figure out whats the issue.

So what was the issue? Do you get the same results if you undo the https://reviews.llvm.org/D51381 and s/CHECK-MESSAGES/CHECK-NOTES/?

You are right that replacing all of it with CHECK-NOTES works as well.
FileCheck is run twice if you have FIXMES as well. Having another run for the notes is consistent with how it worked before.
If we go for the catch-all-with-one approach it might be a good idea to ensure that only one of CHECK-MESSAGES or CHECK-NOTES is present in the file and adjust the check_clang_tidy.py script a little.

I don't have a strong preference, but if that works i would prefer to go that route, since that is what the initial intended semantics of the CHECK-NOTES.
If that works for you?

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D51381

  • adjust check_clang_tidy to use either CHECK-NOTES or CHECK-MESSAGES but not both
JonasToth retitled this revision from [clang-tidy] fix check_clang_tidy to properly mix CHECK-NOTES and CHECK-MESSAGES to [clang-tidy] fix check_clang_tidy to forbid mixing of CHECK-NOTES and CHECK-MESSAGES.Aug 30 2018, 12:51 AM
JonasToth edited the summary of this revision. (Show Details)
lebedev.ri accepted this revision.Aug 30 2018, 12:54 AM

LG, thank you!

This revision is now accepted and ready to land.Aug 30 2018, 12:54 AM
This revision was automatically updated to reflect the committed changes.