jfb added a comment. In https://reviews.llvm.org/D46112#1113557, @aaron.ballman wrote:
> 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? FYI I ran into this here: https://reviews.llvm.org/D49458 https://reviews.llvm.org/D46112 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
