[PATCH] D46112: Allow _Atomic to be specified on incomplete types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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