[PATCH] D147920: [clang] Add test for CWG399

[Vlad Serebrennikov via Phabricator via cfe-commits]

Endill added a comment.

@shafik ping



================
Comment at: clang/test/CXX/drs/dr3xx.cpp:1439
+
+namespace dr399 { // dr399: 11
+                  // NB: reuse dr244 test 
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > shafik wrote:
> > > Endill wrote:
> > > > shafik wrote:
> > > > > Endill wrote:
> > > > > > Despite a couple of FIXME in CWG244 test (out of dozens of examples), it claims full availability since Clang 11. I'd take a more conservative approach, declaring partial support, but I think that declaring different availability for the same test would bring unnecessary confusion. So I followed CWG244 availability.
> > > > > > 
> > > > > > Alternative is to demote CWG244 to partial, but I'm not sure we should go back on our claims for CWG support that has been out for so long.
> > > > > I think the bugs are not awful, we should file bug reports if we don't already have them. Some of them seem like they should be not too bad to fix.
> > > > > 
> > > > > CC @aaron.ballman to get a second opinion
> > > > If we are to file bug reports, I'm not sure what wording makes those examples ill-formed. Is it [[ http://eel.is/c++draft/basic.lookup#qual.general-4.6 | qual.general-4.6 ]]: `The type-name that is or contains Q shall refer to its (original) lookup context (ignoring cv-qualification) under the interpretation established by at least one (successful) lookup performed.`? I interpret it as requiring names to the left and to the right of `~` to be found in the same scope (lookup context; `namespace dr244` in our case). Could it actually mean that they have to refer to the same type?
> > > I am not sure maybe @rsmith might be able to help us here.
> > I think we want to start being more conservative with claiming support for features and DRs, and that means being more honest with "partial" markings (with comments as to WHY the support is only partial, what's still left to be done, etc). I don't think it's a problem to say "we've discovered enough issues with this that we no longer claim to support it" when that's accurate.
> > 
> > I don't think we have a hard and fast rule for when a bug is sufficiently worrying to merit partial vs full support; it's going to depend on the situation, I think. Failing to diagnose incorrect code is a different kind of problem from diagnosing correct code from crashing bugs from etc. and it's going to be up to the patch author and reviewers to make a value judgement. But that's why I think it's fine for us to update the status when we learn more information, too.
> > 
> > That said, when we do have partial support, we definitely need to file issues to address the remaining bits at some point.
> > 
> > > I interpret it as requiring names to the left and to the right of ~ to be found in the same scope (lookup context; namespace dr244 in our case). Could it actually mean that they have to refer to the same type?
> > 
> > I've read that wording a few times now and can't make heads or tails of what it's trying to say. Perhaps @rsmith or @hubert.reinterpretcast can help illuminate us?
> @Endill reminded me off-list that the FIXME comments here are existing comments; some of these test cases are lifted from the dr244 test cases. Given that and it's been a few weeks and we've not determine what issues to file, I think we should unblock this review as it makes forward progress on our test coverage for dr399. Filing issues would be good, but not a prerequisite for landing this. WDYT @shafik?
> @Endill reminded me off-list that the FIXME comments here are existing comments; some of these test cases are lifted from the dr244 test cases. Given that and it's been a few weeks and we've not determine what issues to file, I think we should unblock this review as it makes forward progress on our test coverage for dr399. Filing issues would be good, but not a prerequisite for landing this. WDYT @shafik?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147920