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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D46112#1098243, @rsmith wrote:

> 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 :)


It only gets in the way for C++ whereas my primary concern for this patch is C. I tried spending a few hours on this today and got nowhere -- we have a lot of FIXME comments surrounding atomic type qualifiers in C++. I've run out of time to be able to work on this patch and may have to abandon it. I'd hate to lose the forward progress already made, so I'm wondering if the C bits are sufficiently baked that even more FIXMEs around atomics in C++ would suffice?


https://reviews.llvm.org/D46112