[PATCH] D158006: [Clang][WIP]Experimental implementation of data member packs in dependent context.

[Denis Nikitin via Phabricator via cfe-commits]

denik added inline comments.


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:528-529
+        if (Field->getDeclName() == NameInfo.getName()) {
+          if (const PackExpansionType *PET =
+                  dyn_cast<PackExpansionType>(Field->getType())) {
+            isMemberPack = true;
----------------
if (isa<PackExpansionType>(Field->getType())) { ...


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:1018
   if (R.empty()) {
+    // The member is expaned from member pack if its name has `@` in it. In this
+    // case a failed name lookup is not an error, but represents it's at the end
----------------
expanded


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3289
+          // Rename the expanded fields to avoid ambiguous names in
+          // name lookup. The renamed fields need to be invisible to users so an
+          // `@` is added to the name.
----------------
nit: Maybe `inaccessible by`?


================
Comment at: clang/lib/Sema/TreeTransform.h:4177
+               "trying to expand non-pack member access");
+        std::string UnExpanedNameStr =
+            MemberExpr->getMemberNameInfo().getName().getAsString();
----------------
Expanded


================
Comment at: clang/lib/Sema/TreeTransform.h:4198-4199
+              MemberExpr->getMemberNameInfo().getInfo());
+          TemplateArgumentListInfo TemplateArgs = TemplateArgumentListInfo();
+          MemberExpr->copyTemplateArgumentsInto(TemplateArgs);
+          auto *ExpandedMemberExpr = CXXDependentScopeMemberExpr::Create(
----------------
Why is this in a loop?


================
Comment at: clang/lib/Sema/TreeTransform.h:4208
+
+          Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), Arg);
+          ExprResult Out = getDerived().TransformExpr(ExpandedMemberExpr);
----------------
Can we add a check that Arg never exceeds the number of arguments in the full expansion (is it Expansion->getNumExpansions()?)


================
Comment at: clang/test/CodeGenCXX/data_member_packs.cpp:76
+  // CHECK: double @_Z3sumIJilfdEEDaDpT_(i32 noundef %ts, i64 noundef %ts1, float noundef %ts3, double noundef %ts5)
+  S4<int, long>{}.sum_pack<float, double>(s7);
+  return 0;
----------------
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);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158006