rsmith added a comment. In https://reviews.llvm.org/D46112#1096893, @aaron.ballman wrote:
> In https://reviews.llvm.org/D46112#1091981, @efriedma wrote: > > > Also needs some test coverage for atomic operations which aren't calls, > > like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; > > };". > > > Thank you for pointing this out -- that uncovered an issue where we were not > properly diagnosing the types as being incomplete. I've added a new test case > and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier > patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it). I don't think this has sufficiently addressed the comment. Specifically, for a case like: struct NotTriviallyCopyable { NotTriviallyCopyable(const NotTriviallyCopyable&); }; void f(_Atomic NotTriviallyCopyable *p) { *p = *p; } ... we should reject the assignment, because it would perform an atomic operation on a non-trivially-copyable type. It would probably suffice to check the places where we form an `AtomicToNonAtomic` or `NonAtomicToAtomic` conversion. In passing, I noticed that this crashes Clang (because we fail to create an lvalue-to-rvalue conversion for `x`): struct X {}; void f(_Atomic X *p, X x) { *p = x; } This will likely get in the way of your test cases unless you fix it :) ================ Comment at: lib/Sema/SemaChecking.cpp:3202 - if (!IsC11 && !AtomTy.isTriviallyCopyableType(Context) && - !AtomTy->isScalarType()) { - // For GNU atomics, require a trivially-copyable type. This is not part of - // the GNU atomics specification, but we enforce it for sanity. + if (!ValType.isTriviallyCopyableType(Context) && !ValType->isScalarType()) { + // Always require a trivially-copyable type. This is not part of the GNU ---------------- The (pre-existing) `isScalarType()` check here looks wrong to me. If we had a non-trivially-copyable scalar type (which I think do not currently exist in our type system), I think we should reject it here. ================ Comment at: lib/Sema/SemaType.cpp:7662 // try to instantiate it. - QualType MaybeTemplate = T; - while (const ConstantArrayType *Array - = Context.getAsConstantArrayType(MaybeTemplate)) - MaybeTemplate = Array->getElementType(); - if (const RecordType *Record = MaybeTemplate->getAs<RecordType>()) { + if (auto *RD = dyn_cast_or_null<RecordDecl>(Def)) { bool Instantiated = false; ---------------- May as well cast directly to `CXXRecordDecl` here; none of the code controlled by this `if` does anything in C. https://reviews.llvm.org/D46112 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
