[PATCH] D122656: [C11] Improve the diagnostic when accessing a member of an atomic struct

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:1300
+  // warning defaults to an error.
+  if (const auto *ATy = BaseType->getAs<AtomicType>()) {
+    S.Diag(OpLoc, diag::warn_atomic_member_access);
----------------
erichkeane wrote:
> This seems to apply to both C and C++.  I guess "_Atomic" is C only, so we get to define its behavior for C++?
> 
> What does GCC do in C++ mode?
Clang supports `_Atomic` in C++ as an extension (with the C semantics), GCC does not.

However, after doing some testing, I actually question whether Clang supports `_Atomic` in C++ more or just "pretends everything will work out fine in C++ mode but nobody ever actually checked that". I'm not going to add additional test coverage for this because I'm going to see just how many issues I spot in C++ first (it may be a better approach to disable the extension in C++).


================
Comment at: clang/test/Sema/atomic-expr.c:85
+  x.val = 12; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  xp->val = 12; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+
----------------
erichkeane wrote:
> This still catches RHS access as well, right?  
> 
> Also, does it still 'work' with the non-qualifier version of _Atomic?
> This still catches RHS access as well, right?

Yes, I'll add tests.

> Also, does it still 'work' with the non-qualifier version of _Atomic?

Yes, I'll add a test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122656/new/

https://reviews.llvm.org/D122656