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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
