This is an archive of the discontinued LLVM Phabricator instance.

[Clang][WIP]Experimental implementation of data member packs declarations
Needs ReviewPublic

Authored by SlaterLatiao on Jul 28 2023, 8:06 AM.

Details

Summary
  • Experimental implementation of discourse#71333.
  • Supports declaration of data member packs.
  • Does not support access to the declared members.
  • Known issue: duplicate names in instantiated fields.
  • Known issue: The data member pack declaration is not a pack expansion type. This will be solved by a following differential.

Diff Detail

Event Timeline

SlaterLatiao created this revision.Jul 28 2023, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2023, 8:06 AM
SlaterLatiao requested review of this revision.Jul 28 2023, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2023, 8:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
SlaterLatiao retitled this revision from Experimental implementation of packed data member declaration to [Clang][WIP]Experimental implementation of packed data members declarations.Jul 28 2023, 8:14 AM
SlaterLatiao edited the summary of this revision. (Show Details)

(just a side note about the title of this patch: I got "packed data members" confused and thought this was referring to struct packing __attribute__((packed)) - so perhaps something more like "data member packs" would be a more clear term here?)

clang/lib/Sema/SemaTemplateInstantiate.cpp
3367–3368
3369
clang/test/CodeGenCXX/packed_data_member.cpp
8–12 ↗(On Diff #545169)

Did this test case come out of any particular bug discovered during implementation?

14 ↗(On Diff #545169)

Not sure, but might be worth a test with multiple of the same type in the pack?

27 ↗(On Diff #545169)

(please add the missing newline at the end of the file here)

SlaterLatiao retitled this revision from [Clang][WIP]Experimental implementation of packed data members declarations to [Clang][WIP]Experimental implementation of data member packs declarations.Jul 28 2023, 2:38 PM
SlaterLatiao edited the summary of this revision. (Show Details)
  • Address review comments.
SlaterLatiao marked 2 inline comments as done.

Revert last commit.

Address comments.

  • Added test case of duplicate types in template arguments.
  • Added ending new line to test case file.
  • Renamed "packed data members" -> "data member packs"

(just a side note about the title of this patch: I got "packed data members" confused and thought this was referring to struct packing __attribute__((packed)) - so perhaps something more like "data member packs" would be a more clear term here?)

I changed the name to "data member packs" in description and in the code.

SlaterLatiao marked 2 inline comments as done.Jul 28 2023, 3:41 PM
SlaterLatiao added inline comments.Jul 28 2023, 3:43 PM
clang/test/CodeGenCXX/packed_data_member.cpp
8–12 ↗(On Diff #545169)

No. I just wanted to test the case when the packed parameter is not the only parameter.

dblaikie added inline comments.Jul 31 2023, 9:40 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
3365

Not sure if this is a niche opinion, but the std::optional operator overloads make me a bit uncomfortabel/bit unclear what's happening, and I'd personally put a deref in here to make it clearer that this code is assuming (in addition to the assert above) the optional is set/present.

3375–3376

Is this codepath tested?

3378–3381

Worth having a test case showing that/what's broken here?

clang/lib/Sema/SemaType.cpp
5930–5931

Perhaps a few more words here would be good - quoting a WG21 proposal paper that this code implements, similar to the standard quotations in nearby code. (be good to mention the WG21 proposal paper/whatnot in the patch description/commit message too)

cjdb added inline comments.Jul 31 2023, 10:10 AM
clang/lib/Sema/SemaType.cpp
5930–5931

I would suggest a link to the discourse discussion instead of P1858, since that thread talks about the proposal and goes into a bit more detail than the member packs section of the proposal.

SlaterLatiao marked an inline comment as done.Jul 31 2023, 2:22 PM
SlaterLatiao added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
3378–3381

It's copied from the handling of non-pack members and possibly a duplicate that should be avoided. I'm not sure which case could trigger this branch. Will look into it.

cor3ntin added inline comments.Jul 31 2023, 3:11 PM
clang/lib/Sema/SemaType.cpp
5930–5931

@dblaikie This does not implement a C++ proposal - especially one that would be anywhere near approval (ie we had vague talk that WG21 want something in that space in P1858 and related papers, but no actual specification or consensus of any kind) and this PR also does not implement or have proposals for all the complications this features has in terms of accessing such member packs
(ie packs in non dependent context, disambiguation between member of a variadic variable and pack members and combinations of the two)

This is a very interesting experiment, but we should be careful not to approve an extension that is going to conflict with future C++ language features

cjdb added inline comments.Aug 1 2023, 1:51 PM
clang/lib/Sema/SemaTemplateInstantiate.cpp
3356–3368

In order to help improve the readability of this function, I suggest breaking the branches into two separate functions.

  • Address review comments.
SlaterLatiao marked an inline comment as done.Aug 1 2023, 4:20 PM
SlaterLatiao added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
3375–3376

Some checks in Visit need to be implemented for this branch. I elaborated in the comment and will implement in a future differential.

3378–3381

We Traced back and could not find a test case associated with this block. I added a cross reference of this to the FIXME below.

  • Refactor instantiating members into 2 functions.
SlaterLatiao marked an inline comment as done.Aug 16 2023, 4:14 PM