This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add tests for DRs about complete-class context
Needs ReviewPublic

Authored by Endill on Apr 15 2023, 9:16 AM.

Details

Reviewers
erichkeane
shafik
Group Reviewers
Restricted Project
Summary

P1787: The intent for CWG2335 (contra those of the older CWG1890, CWG1626, CWG1255, and CWG287) is supported by retaining the unrestricted forward lookup in complete-class contexts (despite current implementation behavior for non-templates).
Wording: The declaration set is the result of a single search in the scope of C for N from immediately after the class-specifier of C if P is in a complete-class context of C or from P otherwise. [Drafting note: The plan for CWG2335 is to describe forbidden dependency cycles among the complete-class contexts of a class. — end drafting note] ([class.member.lookup]/4)

Complete-class context is described in [class.mem.general] p7 and p8. In this patch I add tests only for CWG issues that fall under current definition of complete-class context, because I'm not sure how CWG1255 and CWG287 are going to work. That's why I skip over them, but mark CWG1308 as superseded by CWG1330.

Diff Detail

Event Timeline

Endill created this revision.Apr 15 2023, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2023, 9:16 AM
Endill requested review of this revision.Apr 15 2023, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2023, 9:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Endill added a reviewer: Restricted Project.Apr 15 2023, 9:32 AM
shafik added inline comments.
clang/test/CXX/drs/dr18xx.cpp
139

So this is still in drafting: https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1890

We saw this in Issaquah and we felt like 1890 would probably be resolved once we resolve 2335: https://www.open-std.org/JTC1/SC22/WG21/docs/cwg_active.html#2335

I think we are leaning towards these being well-formed but we won't really know till it is resolved.

clang/test/CXX/drs/dr23xx.cpp
42

My comment on 1890 applies here as well.

CC @rsmith @aaron.ballman how should we handle DRs that are still in process? While we may think we know the direction it is going in, it could change.

So maybe we should avoid expressing an opinion on whether these are well-formed or not?

shafik added inline comments.Apr 17 2023, 8:56 AM
clang/test/CXX/drs/dr16xx.cpp
46

Since this is still open, should we be expressing an opinion on whether the examples are all well-formed or not?

Endill added inline comments.Apr 17 2023, 9:15 AM
clang/test/CXX/drs/dr23xx.cpp
42

I asked Aaron even before uploading the patch. His response was:

I think it's a judgement call -- if the CWG consensus seems like it makes a lot of sense, then I see no reason not to test them but maybe leave a FIXME comment about the issue technically being open still. If the CWG consensus doesn't make sense, that might be time to get on the reflectors and ask questions

Consensus documented in 2335 and drafting notes in P1787 that I quote in the summary made sense to me, so I published this patch. I can abandon it if there are fundamental issues with the them, rendering my judgement wrong.

aaron.ballman added inline comments.Apr 17 2023, 9:50 AM
clang/test/CXX/drs/dr23xx.cpp
42

I'd be curious to hear more thoughts on this.

In this particular case, this has been in drafting status since 2017, but the final comments on the open issue are:

Notes from the June, 2018 meeting:

The consensus of CWG was to treat templates and classes the same by "instantiating" delayed-parse regions when they are needed instead of at the end of the class.

See also issue 1890.

which seemed sufficiently firm in direction to warrant testing the behavior. I think any open DR being tested is subject to change in Clang and should continue to be documented in cxx_dr_status.html as "not resolved", though (so if CWG does make a change, we can still react to it without having promised the current behavior to users).

Endill added inline comments.Apr 17 2023, 10:16 AM
clang/test/CXX/drs/dr23xx.cpp
42

As a recap, in D138901 I updated make_cxx_dr_status script, so that it can take into account unresolved issues. Precedents were 2565 and 2628, which resorted to editing cxx_dr_status.html manually. Script now make sure that open or drafting bit in the comment matches status from the official page, throwing an exception otherwise.

shafik added inline comments.Apr 17 2023, 11:59 AM
clang/test/CXX/drs/dr23xx.cpp
42

I think documenting this is important.

What Aaron says makes sense to me, I just find the current comment all of the examples are well-formed. feels like a promise even though in the status page we may say not resolved.

I feel like the wording should be more cautious, maybe more like Current consensus is that the examples are well-formed.

Endill updated this revision to Diff 514353.Apr 17 2023, 12:09 PM

Weaken the FIXME wording per Shafik's suggestion