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

[SlaterLatiao]

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.

[dblaikie]

(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

3281–3282

3283

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]

• Address review comments.

[SlaterLatiao]

Revert last commit.

[SlaterLatiao]

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"

[SlaterLatiao]

In D156546#4543697, @dblaikie wrote:

(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]

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]

clang/lib/Sema/SemaTemplateInstantiate.cpp

3279

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.

3289–3290

Is this codepath tested?

3292–3295

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]

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]

Fix code.

[SlaterLatiao]

clang/lib/Sema/SemaTemplateInstantiate.cpp

3292–3295

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]

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]

clang/lib/Sema/SemaTemplateInstantiate.cpp

3270–3346

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

[SlaterLatiao]

• Address review comments.

[SlaterLatiao]

clang/lib/Sema/SemaTemplateInstantiate.cpp

3289–3290

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

3292–3295

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.

[SlaterLatiao]

• Refactor instantiating members into 2 functions.

clang/lib/Sema/SemaTemplateInstantiate.cpp

if (isa<BlockDecl>(Member) ||
         (isa<CXXRecordDecl>(Member) && cast<CXXRecordDecl>(Member)->isLambda()))
       continue;
 
     if (Member->isInvalidDecl()) {
       Instantiation->setInvalidDecl();
       continue;
     }
 
+    // Instantiate packed data members.
+    if (FieldDecl *Field = dyn_cast<FieldDecl>(Member);
+        Field && isa<PackExpansionType>(Field->getType().getTypePtr())) {
+      QualType PatternType = Field->getType()
+                                 ->castAs<PackExpansionType>()
+                                 ->getPattern();
+      std::optional<unsigned> NumArgumentsInExpansion =
+          getNumArgumentsInExpansion(Field->getType(), TemplateArgs);
+      assert(NumArgumentsInExpansion && "should not see unknown template argument here");
+      for (unsigned Arg = 0; Arg < *NumArgumentsInExpansion; ++Arg) {
+        // Generate a new field from PackExpansion field.
+        if (Decl *NewMember = Instantiator.Visit(Member)) {
+          FieldDecl *PackedField = cast<FieldDecl>(NewMember);
+          Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(*this, Arg);
+          QualType T =
+              SubstType(PatternType, TemplateArgs, PackedField->getLocation(),
+                        PackedField->getDeclName());
+          PackedField->setType(T);
+          Fields.push_back(PackedField);
+          if (NewMember->isInvalidDecl()) {
+            // When `NewMember` has type of `PackExpansionType`, it escapes
+            // validation checks in `Visit`. Handling of such cases
+            // will be implemented in a future commit.
+            // Currently this branch should never be reached.
+            assert(false && "not implemented");
+            Instantiation->setInvalidDecl();
+          }
+        } else {
+          // FIXME: This is the same situation of the next FIXME, when handling
+          // non-pack members.
+          continue;
+        }
+      }
+    } else {
       Decl *NewMember = Instantiator.Visit(Member);
       if (NewMember) {
         if (FieldDecl *Field = dyn_cast<FieldDecl>(NewMember)) {
           Fields.push_back(Field);
         } else if (EnumDecl *Enum = dyn_cast<EnumDecl>(NewMember)) {
           // C++11 [temp.inst]p1: The implicit instantiation of a class template
           // specialization causes the implicit instantiation of the definitions
           // of unscoped member enumerations.
           // Record a point of instantiation for this implicit instantiation.
           if (TSK == TSK_ImplicitInstantiation && !Enum->isScoped() &&
               Enum->isCompleteDefinition()) {
-          MemberSpecializationInfo *MSInfo =Enum->getMemberSpecializationInfo();
+            MemberSpecializationInfo *MSInfo =
+                Enum->getMemberSpecializationInfo();
             assert(MSInfo && "no spec info for member enum specialization");
             MSInfo->setTemplateSpecializationKind(TSK_ImplicitInstantiation);
             MSInfo->setPointOfInstantiation(PointOfInstantiation);
           }
-      } else if (StaticAssertDecl *SA = dyn_cast<StaticAssertDecl>(NewMember)) {
+        } else if (StaticAssertDecl *SA =
+                       dyn_cast<StaticAssertDecl>(NewMember)) {
           if (SA->isFailed()) {
             // A static_assert failed. Bail out; instantiating this
             // class is probably not meaningful.
             Instantiation->setInvalidDecl();
             break;
           }
         } else if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewMember)) {
           if (MD->isConstexpr() && !MD->getFriendObjectKind() &&
               (MD->isVirtualAsWritten() || Instantiation->getNumBases()))
             MightHaveConstexprVirtualFunctions = true;
         }
 
         if (NewMember->isInvalidDecl())
           Instantiation->setInvalidDecl();
       } else {
         // FIXME: Eventually, a NULL return will mean that one of the
         // instantiations was a semantic disaster, and we'll want to mark the
         // declaration invalid.
         // For now, we expect to skip some members that we can't yet handle.
+        // Same situation occurs when handling member packs, in the previous
+        // FIXME.
+      }
     }
   }
 
   // Finish checking fields.
   ActOnFields(nullptr, Instantiation->getLocation(), Instantiation, Fields,
               SourceLocation(), SourceLocation(), ParsedAttributesView());
   CheckCompletedCXXClass(nullptr, Instantiation);
 
   // Default arguments are parsed, if not instantiated. We can go instantiate
   // default arg exprs for default constructors if necessary now. Unless we're

clang/lib/Sema/SemaType.cpp

case DeclaratorContext::RequiresExpr:
              diag::err_function_parameter_pack_without_parameter_packs)
           << T <<  D.getSourceRange();
         D.setEllipsisLoc(SourceLocation());
       } else {
         T = Context.getPackExpansionType(T, std::nullopt,
                                          /*ExpectPackInType=*/false);
       }
       break;
+    case DeclaratorContext::Member:
+      // Expand for data member packs.
+      // https://discourse.llvm.org/t/adding-support-for-data-member-packs/71333
     case DeclaratorContext::TemplateParam:
       // C++0x [temp.param]p15:
       //   If a template-parameter is a [...] is a parameter-declaration that
       //   declares a parameter pack (8.3.5), then the template-parameter is a
       //   template parameter pack (14.5.3).
       //
       // Note: core issue 778 clarifies that, if there are any unexpanded
       // parameter packs in the type of the non-type template parameter, then
if (D.hasEllipsis()) {
     case DeclaratorContext::KNRTypeList:
     case DeclaratorContext::ObjCParameter: // FIXME: special diagnostic here?
     case DeclaratorContext::ObjCResult:    // FIXME: special diagnostic here?
     case DeclaratorContext::TypeName:
     case DeclaratorContext::FunctionalCast:
     case DeclaratorContext::CXXNew:
     case DeclaratorContext::AliasDecl:
     case DeclaratorContext::AliasTemplate:
-    case DeclaratorContext::Member:
     case DeclaratorContext::Block:
     case DeclaratorContext::ForInit:
     case DeclaratorContext::SelectionInit:
     case DeclaratorContext::Condition:
     case DeclaratorContext::CXXCatch:
     case DeclaratorContext::ObjCCatch:
     case DeclaratorContext::BlockLiteral:
     case DeclaratorContext::LambdaExpr:

clang/test/CodeGenCXX/data_member_packs.cpp

+// RUN: %clang_cc1 --std=c++20 %s -emit-llvm -o - -triple x86_64-linux | FileCheck %s --check-prefixes=CHECK
+
+// Tests declaration data member packs.
+template<typename... Ts> struct S1 {
+    Ts... ts;
+};
+
+template<typename T, typename... Ts> struct S2 {
+    T t[2];
+    Ts... ts;
+};
+
+// CHECK: %struct.S1 = type { i32 }
+S1<int> s1;
+// CHECK-NEXT: %struct.S1.0 = type { i32, float, double }
+S1<int, float, double> s2;
+// Test template args as the last arg.
+// CHECK-NEXT: %struct.S2 = type { [2 x i32], float, double }
+S2<int, float, double> s3;
+// Test nested template args.
+// CHECK-NEXT: %struct.S1.1 = type { i32, float, %struct.S1.2 }
+// CHECK-NEXT: %struct.S1.2 = type { double, double }
+S1<int, float, S1<double, double>> s4;
+// Test empty template arg.
+// CHECK-NEXT: %struct.S1.3 = type { i8 }
+S1<> s5;
+// Test duplicate types in template args.
+// CHECK-NEXT: %struct.S1.4 = type { i32, i32 }
+S1<int, int> s6;
+