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

Thu Sep 30 20:00:27 PDT 2021

FreddyYe added a comment.

> 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!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110798