- 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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
(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) |
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"
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. |
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) |
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. |
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. |
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 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 |
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. |
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. |
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.