This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Use CHECK-NEXT instead of CHECK-SAME in target-invalid-cpu-note.c
ClosedPublic

Authored by FreddyYe on Sep 29 2021, 7:19 PM.

Diff Detail

Event Timeline

FreddyYe created this revision.Sep 29 2021, 7:19 PM
FreddyYe requested review of this revision.Sep 29 2021, 7:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 7:19 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I found the old way cannot verify if there are some extra outputs between two different CHECK-SAME. So I changed to CHECK-NEXT. But it will introduce bad format issue. Anyway, the old way has broken clang-format already. So I would prefer the CHECK-NEXT. WDYT?

I found the old way cannot verify if there are some extra outputs between two different CHECK-SAME. So I changed to CHECK-NEXT.

In principle I agree but did you have this failure mode actually happen?

But it will introduce bad format issue. Anyway, the old way has broken clang-format already. So I would prefer the CHECK-NEXT. WDYT?

Not sure I like crazy long lines, but I see that -NEXT then -SAME would fall to the same issue.

Arm targets are just checking that we print *some* list of CPUs, others are putting the full list. Which isn't great because if you add a new CPU it's possible you'll not get a failure here. I looked for other tests that might check the exact set of CPUs but this is the only one.

I think a reasonable compromise is to -NEXT the note: valid target CPU values are: <at least one cpu name> then -SAME the rest. Check the last line ends in {{$}}. That limits where extra stuff can sneak in and means you can read the file and it's failure output more easily. (each -SAME line has multiple CPUs on it so that limits how much can be missed)

If you feel like adding the full CPU list to the Arm targets go ahead.

Anyway, the old way has broken clang-format already.

What does clang-format complain about? This is a test file so formatting is less of a concern than being readable for maintainers and having useful FileCheck output. Splitting the matches enables that.

FreddyYe added inline comments.Sep 30 2021, 7:49 PM
clang/test/Misc/target-invalid-cpu-note.c
33

I forgot to give an example, sorry. For example, if I delete the last target-cpu bonnell, here, this lit test can still pass. And if I delete any of the first or the last target-cpu on each of -SAME line in this file, the test can still pass. That is my concern. Fortunately, when I changed this file to -NEXT, no new fails happen this time, which means no missing CPUs between different -SAME in the old file.

In principle I agree but did you have this failure mode actually happen?

No failure happens for now, but may happen in the future if we continue to use -SAME. Pls read the example I gave in last comment.

Not sure I like crazy long lines, but I see that -NEXT then -SAME would fall to the same issue.

Arm targets are just checking that we print *some* list of CPUs, others are putting the full list. Which isn't great because if you add a new CPU it's possible you'll not get a failure here. I looked for other tests that might check the exact set of CPUs but this is the only one.

Yeah, I suppose this is the only one test to check this valid CPU list? Then I suppose to add check whole list for Arm targets.

I think a reasonable compromise is to -NEXT the note: valid target CPU values are: <at least one cpu name> then -SAME the rest. Check the last line ends in {{$}}. That limits where extra stuff can sneak in and means you can read the file and it's failure output more easily. (each -SAME line has multiple CPUs on it so that limits how much can be missed)

Can you read the latest example I comment? I think you misunderstand the extra outputs I mentioned. Or if I'm wrong, can you give an inline example?

If you feel like adding the full CPU list to the Arm targets go ahead.

Good to know your thoughts! I'll do.

What does clang-format complain about? This is a test file so formatting is less of a concern than being readable for maintainers and having useful FileCheck output. Splitting the matches enables that.

Sorry I didn't realize before the fact you mentioned here, let's ignore that comment.

Thanks for your review. Helped a lot!

Can you read the latest example I comment? I think you misunderstand the extra outputs I mentioned. Or if I'm wrong, can you give an inline example?

I understood but yes your example is one that a SAME cannot catch, that's true. My point was that SAME does catch some changes, it's not like it's useless.

The only drawback to a single NEXT is when it fails the output from FileCheck isn't useful until you manually compare the lines it found, since they're so long. Then again a lot of FileCheck output can be like that and using NEXT makes the test more robust so sure, let's go with NEXT.

Yeah, I suppose this is the only one test to check this valid CPU list? Then I suppose to add check whole list for Arm targets.

Correct. (though I think downstream Arm Compiler tests this in other ways but that's beside the point)

clang/test/Misc/target-invalid-cpu-note.c
2

Please add a comment as the first line explaining that why we use NEXT and check {{$}} ending.

28

This one uses {{$}} which I think (please check) means that anything extra on the end of the line will be a mistmatch. I'd add that to the rest as well.

FreddyYe marked an inline comment as done.Oct 7 2021, 7:16 PM

Sorry for late response. Thanks for your comments, @DavidSpickett !

clang/test/Misc/target-invalid-cpu-note.c
28

Yes, you are right. When adding the {{$}}. I found two targets missing some checks for cpus at the end of the line: NVPTX and HEXAGON. So adding this is helpful.

FreddyYe updated this revision to Diff 378065.Oct 7 2021, 7:17 PM

add {{$}}. add comments at first line.

This LGTM with the comment reworded. Thanks!

clang/test/Misc/target-invalid-cpu-note.c
2

This wording is a bit confusing, how about:
// Use CHECK-NEXT instead of multiple CHECK-SAME to ensure we will fail if there is anything extra in the output

FreddyYe updated this revision to Diff 378199.Oct 8 2021, 6:49 AM
FreddyYe marked an inline comment as done.

Address comments.

clang/test/Misc/target-invalid-cpu-note.c
2

Looks better. Thanks!

DavidSpickett accepted this revision.Oct 8 2021, 6:53 AM

Forgot to accept, looks good.

This revision is now accepted and ready to land.Oct 8 2021, 6:53 AM
This revision was landed with ongoing or failed builds.Oct 8 2021, 7:08 AM
This revision was automatically updated to reflect the committed changes.