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