[clang] Reland "[Utils] add update-verify-tests.py" (#108630)" (PR #108658)

[Henrik G. Olsson via cfe-commits]

hnrklssn 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. When test authors actively want to do that the can always use the regex feature, but I think the default should be to match the exact string. When there are similar diagnostics or shorter/longer version it's too much of a footgun to accidentally have shorter diagnostic match the longer version as well. When I review, i prefer the test to give me a picture as close to the exact output as possible, so I can evaluate the user friendliness. I think it's a massive shortcoming that we don't check the order in which notes are emitted, or which range is highlighted, because reviewers don't get the full picture without checking out the branch and manually compiling test cases.

> Finally, as @Endilll pointed out, there are tests which have multiple (sometime numerous) RUN lines where the output is expected to differ between RUN lines; we typically ask authors to use custom verify prefixes for those tests, but crafting those can sometimes be an art form (and other times is very straightforward).

Again, this script doesn't run on tests with multiple prefixes. It's meant for test cases which are trivial to update, of which there are many.

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

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

> So another huge side note: I've been pushing REALLY hard for us to use the bookmark feature more aggressively for notes. So I've been asking authors to do THAT as well.

I appreciate that, it can be really tricky to get a feel for the flow of notes, especially when a diagnostic has multiple notes attached. The test sadly doesn't check that the order of the notes emitted is actually sensical, for example. I don't know the solution, but I think there is some inherent flaw when it comes to how `-verify` tests operate currently, in that you need to trust that the author formatted the test to accurately reflect reality.

https://github.com/llvm/llvm-project/pull/108658