[PATCH] D138822: [clang] Add test for CWG36

[Erich Keane via Phabricator via cfe-commits]

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