gribozavr2 added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10325
- for (const auto *FD : RD.fields()) {
- // Ill-formed if the field is an ObjectiveC pointer or of a type that is
- // non-trivial for the purpose of calls.
- QualType FT = FD->getType();
- if (FT.getObjCLifetime() == Qualifiers::OCL_Weak) {
- PrintDiagAndRemoveAttr(4);
- return;
- }
-
- if (const auto *RT = FT->getBaseElementTypeUnsafe()->getAs<RecordType>())
- if (!RT->isDependentType() &&
- !cast<CXXRecordDecl>(RT->getDecl())->canPassInRegisters()) {
- PrintDiagAndRemoveAttr(5);
+ std::queue<const RecordDecl *> RecordsWithFieldsToCheck;
+ RecordsWithFieldsToCheck.push(&RD);
----------------
It looks like we don't need to visit the fields in a specific order. If that's
the case, please use SmallVector and pop_back_val. (See
AnalyzeImplicitConversions for an example of a typical worklist algorithm.)
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10330
+ // Ill-formed if the field is an ObjectiveC pointer or of a type that is
+ // non-trivial for the purpose of calls.
+ QualType FT = FD->getType();
----------------
Please split off the part of the comment that talks about "non-trivial for the
purpose of calls", move it to the newly added if statement below, and enhance
the comment to explain the new rule about anonymous unions.
================
Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:201-202
+
+ // `S2` is like `S1`, but `field` is wrapped in an anonymous union (and it
+ // seems that trivial relocatability of `S1` and `S2` should be the same).
+ //
----------------
================
Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:232
+ // `S2` and `S3` examples above show that `[[clang::trivial_abi]]`
+ // *implicitly* propagates into anonymous union and structs. The example
+ // below shows that it is still *not* okay to *explicitly* apply
----------------
That's not exactly true, we don't propagate the trivial_abi attribute onto the
union type. After this patch, the type of the union (which is impossible to get
ahold of) is still non-trivial.
What we do though, is that we ignore the trivial relocatability of the union
itself, and instead look at the members.
================
Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:251
+ // Like `S2`, but the field of the anonymous union is *not* trivial.
+ struct [[clang::trivial_abi]] S5 { // expected-warning {{'trivial_abi'
cannot be applied to 'S5'}} expected-note {{trivial_abi' is disallowed on 'S5'
because it has a field of a non-trivial class type}}
+ S5(S5&& other) {}
----------------
Could you replace the numbers in S1-S5 with more meaningful phrases?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155895/new/
https://reviews.llvm.org/D155895
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
