[PATCH] D139095: [clang] Mark CWG405 as a duplicate of CWG218

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D139095#3964181 <https://reviews.llvm.org/D139095#3964181>, @Endill wrote:

>> I don't think we should mark it as a dup -- we want the status in our tests to match the status on the official document, otherwise things get confusing.
>
> We can do it the following way then: `// dr405: yes \n // NB: also dup 218`.

That would be fine by me!

> Do I understand correctly that superseded status should be used if and only if it's used in official document as well?

That one is somewhat harder in that it's possible for future changes to the standard to supersede previously resolved issues without the committee going back to mark those old DRs as superseded. However, I think in general, we should stick with the status in the document, and we can use extra comments to add our own information on top of it.

>> The two issues are very closely related, but they change different words in the standard and should be tested independently as best we can
>
> Make no mistake here: proposed wording for 405 has never made it into the standard. I double-checked this with revision 100 of CWG issues, when this issue still had open status.
> P1787 <https://reviews.llvm.org/P1787> states that //CWG405 is resolved by stating that argument-dependent lookup (sometimes) occurs after an ordinary unqualified lookup (making statements like “finding a variable prevents argument-dependent lookup” formally correct).//
> The only relevant wording in P1787 <https://reviews.llvm.org/P1787> I can find is for [basic.lookup.argdep] p1 and p3 (too long; not citing them here). Which, curiously enough, clearly originates from resolution of 218, which is already tested. It also has the same intent as proposed resolution for 405.
>
> So I'd like to raise a couple of questions:
>
> 1. What test for 405 is going to be if not a copy-and-paste of a part of 218 test?

In terms of test *coverage*, no benefit. In terms of *implementation status*, it makes it clear we considered the DR explicitly instead of leaving future folks to wonder.

In D139095#3966438 <https://reviews.llvm.org/D139095#3966438>, @Endill wrote:

>> I think this is perfectly fine to have a duplicated test case, I agree with Aaron, we should not invent duplicated status ourselves.
>> Adding a comment in the test like "Note: this test is identical to the one for CWG405" would be a good idea
>
> Does it mean that duplication with cross-references is the best way to handle this even hypothetically? As opposed to, say, new notation for make_cxx_dr_status like `// dr405: dup 405 unofficial`.

I don't see a need for a new notation yet. These notations are specific to us generating the dr status page with the correct information, and users don't usually care about whether two DRs are dupes of one another so much as "I think this DR applies to my code, does Clang implement it?" kind of questions.

>> Nah, that wouldn't be worth the hassle, even if you got people to agree on the duplicated nature
>
> Sad but definitely not unexpected.
>
>> You could do a codegen tests and check that the correct function gets called using its mangled name. There are examples in the drs tests already, grep for "// CHECK: call"
>
> Thanks for mentioning this! Could definitely be used as a last resort. I'll try it for some of the subsequent CWG test.
> Observing front-end behavior via back-end still doesn't feel good, though. I believe debug facilities should be improved to contain as much DR checks as possible at source and AST level.

CodeGen tests would be the approach I'd take; that's not actually testing the backend behavior, that's still testing the frontend IR generation (which is before the backend gets to start mutating/optimizing it).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139095/new/

https://reviews.llvm.org/D139095