nickdesaulniers added inline comments.
================
Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:12
foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh
no foo}}
+ // expected-note@* {{In function 'baz'}}
if (x())
----------------
cjdb wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > Instead of allowing this note to appear anywhere in the file, I think
> > > > > it's better to use "bookmark" comments. e.g.,
> > > > > ```
> > > > > void baz() { // #baz_defn
> > > > > }
> > > > >
> > > > > void foo() {
> > > > > baz(); // expected-note@#baz_defn {{whatever note text}}
> > > > > }
> > > > > ```
> > > > > so that we're testing where the diagnostic is emitted. (This is
> > > > > especially important given that the changes are about location
> > > > > tracking.)
> > > > oh, that's new (to me)! will do
> > > It looks like the "bookmarks" don't work because the notes do not
> > > line+col info. They follow the warning/error diagnostic which does, on
> > > the bottom most call site.
> > >
> > > The warning is supposed to be emitted on the callsite, not the definition.
> > I still don't think this is reasonable for test coverage because this means
> > we'll accept the note *anywhere* in the file. This makes it much too easy
> > to regress the behavior accidentally (e.g., accidentally emit all the notes
> > on line 1 of the file). I think we need *some* way to nail down where these
> > notes are emitted in the test. However, I might be asking you to solve
> > "please track location information through the backend" for that, so
> > perhaps this isn't a reasonable request?
> >
> > Also, CC @cjdb for awareness of how this kind of diagnostic is produced
> > (Chris is working on an idea for how we emit diagnostics so we get better
> > structured information from them, so knowing about this novel approach
> > might impact that idea).
> Very interesting, thanks for the heads up! Cross-phase diagnostics are
> definitely something I hadn't considered, and it **does** impact the design
> (but not in a derailing way).
I think we're back at "please track location information through the backend".
That is the tradeoff with this approach; not measurably regressing performance
when these attributes aren't used in source in exchange for Note diagnostics
that lack precise line+col info, but in practice provide enough info for the
user to understand what's going wrong where.
Your call if the tradeoff is worth it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141451/new/
https://reviews.llvm.org/D141451
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
