This is an archive of the discontinued LLVM Phabricator instance.

[Clang][WIP]Experimental implementation of data member packs in dependent context.
Needs ReviewPublic

Authored by SlaterLatiao on Aug 15 2023, 11:05 AM.

Details

Summary
  • Experimental implementation of discourse#71333.
  • Supports access data member packs in dependent context.
  • Expands access to member packs on transforming member access expression.
  • Renames expanded field names to avoid duplication.

Depends on D156546

Diff Detail

Event Timeline

SlaterLatiao created this revision.Aug 15 2023, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2023, 11:05 AM
SlaterLatiao requested review of this revision.Aug 15 2023, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2023, 11:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
SlaterLatiao retitled this revision from Member pack access in dependent context. to [Clang][WIP]Experimental implementation of data member packs in dependent context..Aug 15 2023, 11:13 AM
SlaterLatiao edited the summary of this revision. (Show Details)
cjdb added inline comments.Aug 15 2023, 1:29 PM
clang/lib/Sema/SemaTemplateInstantiate.cpp
3370

Does LLVM have some form of absl::StrCat that we can use instead of operator+?

clang/lib/Sema/TreeTransform.h
4247–4274

It may be worth putting this into its own named function to help with readability.

clang/test/CodeGenCXX/data_member_packs.cpp
73

This needs to be passed to one of the sum functions and checked that it's generating the correct code.

SlaterLatiao edited the summary of this revision. (Show Details)Aug 15 2023, 1:43 PM
denik added inline comments.Aug 15 2023, 5:02 PM
clang/lib/Sema/SemaExprMember.cpp
528–529

if (isa<PackExpansionType>(Field->getType())) { ...

1012

expanded

clang/lib/Sema/SemaTemplateInstantiate.cpp
3367

nit: Maybe inaccessible by?

clang/lib/Sema/TreeTransform.h
4233

Expanded

4254–4255

Why is this in a loop?

4264

Can we add a check that Arg never exceeds the number of arguments in the full expansion (is it Expansion->getNumExpansions()?)

clang/test/CodeGenCXX/data_member_packs.cpp
76

This is a good test case!
I would also add a test for the partial expansion of the member pack. And also checking the right order of the fields. Something like:

template<typename T> constexpr auto foo(T t) { return t; }
template<typename T, typename ... Ts> constexpr auto foo(T t, Ts ... ts) { return t + 2 * foo(ts...); }

S1<int, int, int> s = {1,2,3};
static_assert(foo(s.ts...) == 17);
MaskRay added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
3370

(Twine(...) + ...).str()

Many times Twine can be omitted.

clang/lib/Sema/TreeTransform.h
4274
dblaikie added inline comments.Aug 16 2023, 3:19 PM
clang/lib/Sema/SemaExprMember.cpp
519–521

I think you can skip the const in the first dyn_cast (think it works implicitly?) & could use const auto * on the LHS of both these ifs, since the RHS makes it clear what the type is

523–524

No need for the assert if you're immediately going to cast anyway, it'll assert. Though perhaps the custom assert here gives you a chance to make it more explicit that this is intentional.

525–527

This could be llvm::find_if, maybe? Not sure if it'd be tidier, but maybe more legible

1017–1019

Usually don't put {} on single-line scopes in the LLVM codebase.

clang/lib/Sema/SemaTemplateInstantiate.cpp
3370

there's at least llvm::Twine which can avoid manifesting the intermediate strings

clang/lib/Sema/TreeTransform.h
4227

Might be worth pulling out this new code as a separate function - the continue at the end is many lines away from the start or end of the loop, making control flow a bit hard to follow (probably the existing code could do with some of this massaging even before/regardless of the new code you're adding here)

Address review comments.

SlaterLatiao marked 6 inline comments as done.Aug 17 2023, 1:39 PM
SlaterLatiao added inline comments.
clang/lib/Sema/SemaExprMember.cpp
523–524

Removed the assert.

525–527

Rewrote with llvm::find_if.

SlaterLatiao marked 2 inline comments as done.

Remove changes in D156546 which doesn't belong to this patch.

  • Minor fix on comment.
  • Replace string '+' with Twine.
SlaterLatiao marked 4 inline comments as done.Aug 17 2023, 5:31 PM
  • Make transforming expressions for member packs a separate function.
  • Minor edits based on review comments.
SlaterLatiao marked 4 inline comments as done.Aug 17 2023, 6:09 PM
SlaterLatiao added inline comments.
clang/lib/Sema/TreeTransform.h
4227

Made the logic a separate function and added a comment to explain the continue.

4254–4255

Each field need to have its own MemberAccessExpression. This loop iterates through the expanded fields.

4264

Expansion->getNumExpansions() returns the number of expansions only when we know it. In this case it returns std::nullopt.
A more legit way to check Arg is to search for the specialization of the struct based on the template args of the current context, but I didn't find a proper way to achieve that.

SlaterLatiao marked an inline comment as done.
  • Added test case for partial expansion.
SlaterLatiao added inline comments.Aug 17 2023, 7:00 PM
clang/test/CodeGenCXX/data_member_packs.cpp
73

It's passed to sum and generation of sum(int, int) is checked.

76

We can't check the runtime state in LLVM/Clang tests. I added a test case for partial expansion and checked the IR. The order of fields can be checked by looking at the order of types in the instantiation of sum.