erichkeane accepted this revision.
erichkeane 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}}
----------------
aaron.ballman wrote:
> 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?
So FWIW, I'm the one pushing to have us use bookmarks more often, which others
haven't done in the past. I found that while reviewing/going through a bunch
of template code for concepts that the tests were 100% unreadable because
bookmarks weren't used.
As far as readability/beginner friendliness: Everyone I've suggested these to
in the past has been a fan of them, and had no problem figuring them out once
sent to the documentation page.
In this case, as Aaron said, it is perhaps not necessary, but I'm still a fan
of keeping them in the order they are emitted if at all possible. If it was up
to me, VerifyDiagnosticsConsumer would enforce diagnostic-note ordering.
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
