[clang] Reland "[Utils] add update-verify-tests.py" (#108630)" (PR #108658)
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 18 09:59:19 PDT 2024
AaronBallman wrote:
> > In fact, we used to have guidance that only the very first instance of the diagnostic should be spelled out fully, and all subsequent ones should be limited to just the bare minimum needed to identify what the diagnostic is. That's why there are hundreds of instances of things like // expected-note {{here}}.
>
> Side note: In general I think the fact that these tests pass if the check implicitly match on a subset of the diagnostic does more harm than good.
+1, it's why I stopped recommending this approach in my reviews.
> > Writing tests is not a mechanical operation, it requires you to think about what you need to test as well as think about how you test it. I think the same is less true for things like testing LLVM IR because that tends to be more routine pattern matching, which typically requires less stylistic choices.
>
> Sometimes it is, sometimes it isn't. Even when it isn't just having the checks added to the test file in approximately the final location for me to move around is a massive boon compared to manually adding them all.
Sure, it's a time-saver when used as intended. I think the push-back is largely coming from recognition that not everyone uses tools as they are intended and we need to balance making it easy to update a large amount of tests (something which occurs infrequently) against the risks of (even accidental) tool misuse.
> > Given the amount of push-back, I think we should probably revert the changes here so we can discuss with a bit wider of an audience than weighed in on the original PR.
>
> Yup, will revert.
Thank you!
https://github.com/llvm/llvm-project/pull/108658
More information about the cfe-commits
mailing list