[PATCH] D156546: [Clang][WIP]Experimental implementation of data member packs declarations

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3279
+      assert(NumArgumentsInExpansion && "should not see unknown template argument here");
+      for (unsigned Arg = 0; Arg < NumArgumentsInExpansion; ++Arg) {
+        // Generate a new field from PackExpansion field.
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3289-3290
+          Fields.push_back(PackedField);
+          if (NewMember->isInvalidDecl())
+            Instantiation->setInvalidDecl();
+        } else {
----------------
Is this codepath tested?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3292-3295
+          // 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.
----------------
Worth having a test case showing that/what's broken here?


================
Comment at: clang/lib/Sema/SemaType.cpp:5930-5931
       break;
+    case DeclaratorContext::Member:
+      // Expand for packed data members.
     case DeclaratorContext::TemplateParam:
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156546