[PATCH] D47618: __c11_atomic_load's _Atomic can be const

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

rsmith added a comment.

We need to figure out what should happen in the OpenCL case, but the rest seems fine.



================
Comment at: lib/Sema/SemaChecking.cpp:3360
     }
-    if (AtomTy.isConstQualified() ||
+    if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
         AtomTy.getAddressSpace() == LangAS::opencl_constant) {
----------------
The `LoadCopy` check is redundant; only the GNU `__atomic_load` builtin has the `LoadCopy` form. But see below, we can avoid this duplicated condition with some refactoring.


================
Comment at: lib/Sema/SemaChecking.cpp:3361
+    if ((Form != Load && Form != LoadCopy && AtomTy.isConstQualified()) ||
         AtomTy.getAddressSpace() == LangAS::opencl_constant) {
       Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_atomic)
----------------
We also need to figure out what to do about this -- should an atomic load from a constant address space be valid? (It seems a little pointless to use an *atomic* load here, but not obviously wrong.)


================
Comment at: lib/Sema/SemaChecking.cpp:3368-3374
   } else if (Form != Load && Form != LoadCopy) {
     if (ValType.isConstQualified()) {
       Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_pointer)
         << Ptr->getType() << Ptr->getSourceRange();
       return ExprError();
     }
   }
----------------
It would be a little nicer to change this `else if` to a plain `if` and conditionalize the diagnostic instead.

Can you track down whoever added the address space check to the C11 atomic path and ask them if they really meant for it to not apply to the GNU atomic builtins?


Repository:
  rC Clang

https://reviews.llvm.org/D47618