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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
