aaron.ballman added inline comments.
================
Comment at: clang/test/CXX/drs/dr0xx.cpp:489
+
+ using B::i; // expected-error {{redeclaration of using declaration}}
+ using C::i; // expected-error {{redeclaration of using declaration}}
----------------
Endill wrote:
> aaron.ballman wrote:
> > Endill wrote:
> > > erichkeane wrote:
> > > > Endill wrote:
> > > > > erichkeane wrote:
> > > > > > As a nit, I prefer the 'notes' to live next to the error, and use a
> > > > > > bookmark line-marker here. My issue is basically how we have no
> > > > > > way of knowing (particularly in template code...) what this
> > > > > > diagnoses.
> > > > > >
> > > > > > I would also think a dependent example of this diagnostic would be
> > > > > > useful.
> > > > > >I would also think a dependent example of this diagnostic would be
> > > > > >useful.
> > > > > Do you mean something of this sort: `using D<int>::i`?
> > > > That is a good example too, but more a case where the using expression
> > > > is dependent, so something like: `using Struct<T>::i` sorta thing
> > > > I prefer the 'notes' to live next to the error
> > > done
> > >
> > > > I would also think a dependent example of this diagnostic would be
> > > > useful.
> > > I'm not sure how you wanted it to interact with virtual bases, so I wrote
> > > examples both with virtual bases and without
> > >
> > > > use a bookmark line-marker here
> > > While I'm sympathetic to your concern, and agree that bookmarks allow to
> > > order expected errors and notes in the order they would appear for user,
> > > searching for `@#` gives only 5 DR tests. If that's the direction we want
> > > DR tests to take, we should be explicit about this, because almost all
> > > existing tests have to be adjusted.
> > > While I'm sympathetic to your concern, and agree that bookmarks allow to
> > > order expected errors and notes in the order they would appear for user,
> > > searching for @# gives only 5 DR tests. If that's the direction we want
> > > DR tests to take, we should be explicit about this, because almost all
> > > existing tests have to be adjusted.
> >
> > FWIW, we don't have to adjust all the tests -- sometimes bookmarks make
> > things more clear, other times they don't help all that much, and it's an
> > equivalent test either way. My recommendation is to use bookmarks when you
> > think they make sense to use or when a reviewer asks for one. Updating
> > other tests can be done with NFC changes if someone sees a particular need.
> > FWIW, we don't have to adjust all the tests -- sometimes bookmarks make
> > things more clear, other times they don't help all that much, and it's an
> > equivalent test either way. My recommendation is to use bookmarks when you
> > think they make sense to use or when a reviewer asks for one. Updating
> > other tests can be done with NFC changes if someone sees a particular need.
>
> The concern about expected-note comments scattered across a test seems
> applicable to any test with more than one expected-warning or expected-error.
> If maintainers agree that it should be addressed with bookmarks, I'll update
> this patch, and make NFC commits to fix existing tests.
>
> What I'd like to avoid is introducing even more inconsistencies into DR
> tests. Until very recently (before tests for 2565 and 2628), bookmarks have
> been used only under `#if __cplusplus`, where you don't have any other
> straightforward option (e.g. dr100).
> The concern about expected-note comments scattered across a test seems
> applicable to any test with more than one expected-warning or expected-error.
To me, it's less about the number of expected diagnostics and more about
locality of the diagnostics. If the warning/error and note are only a few lines
away from one another, I (personally) don't use bookmarks because the syntax is
a bit novel, especially for newcomers. But if there is quite a bit of space
between the warning/error and the note, then I like using bookmarks because it
helps me to pair the note with the diagnostic. e.g.,
```
// To me, this is fine.
int note_goes_here; // expected-note {{the bad thing was declared here}}
foo(note_goes_here); // expected-warning {{something went wrong with this
argument}}
```
```
// To me, this is also fine.
int note_goes_here; // #bad_thing
// ... lots of code ...
// ... even more code ...
foo(note_goes_here); // expected-warning {{something went wrong with this
argument}} \
expected-note@#bad_thing {{the bad thing was declared
here}}
```
but it's a value judgment as to what "quite a bit of space" is and whether
using the bookmark improves the test or not. So:
```
// To me, this is not really a good use of bookmarks.
int note_goes_here; // #bad_thing
foo(note_goes_here); // expected-warning {{something went wrong with this
argument}} \
expected-note@#bad_thing {{the bad thing was declared
here}}
```
> What I'd like to avoid is introducing even more inconsistencies into DR
> tests. Until very recently (before tests for 2565 and 2628), bookmarks have
> been used only under #if __cplusplus, where you don't have any other
> straightforward option (e.g. dr100).
I'm not at all worried about inconsistency in how we surface the `expected`
comments. Bookmarks are a newer and somewhat under-utilized feature of the
diagnostic verifier. We're going to have a lot of tests that predate the
ability to use that feature, so inconsistency is inevitable.
In this particular case, I don't think the bookmarks are all that helpful
because the note and the diagnostic are adjacent in the code. @erichkeane, WDYT?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138822/new/
https://reviews.llvm.org/D138822
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
