[PATCH] Diagnose const atomics and allow more qualified arguments in __atomic builtins.

[Richard Smith]

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6130
@@ -6128,1 +6129,3 @@
+  "address argument to atomic operation must be a pointer to non-const "
+  "type %0 (%1 invalid)">;
 def err_atomic_op_needs_trivial_copy : Error<
----------------
I don't think the `%0` here is really adding anything to the diagnostic; if anything, it seems to distract from the point by making the user wonder if there's a difference between their `%1` and the expected `%0`.

================
Comment at: lib/Sema/SemaChecking.cpp:1411-1415
@@ +1410,7 @@
+static Qualifiers getPointeeQualifiers(Sema& S, Expr *ArgPtr) {
+  ArgPtr = S.DefaultFunctionArrayLvalueConversion(ArgPtr).get();
+  QualType Ty = ArgPtr->getType();
+  const PointerType *PointerTy = Ty->getAs<PointerType>();
+  return PointerTy ? PointerTy->getPointeeType().getQualifiers()
+                   : Qualifiers();
+}
----------------
It's wasteful to build a converted expression up here and throw it away. What you want seems to be something like:

  QualType T = ArgPtr->getType();
  auto *PtrType = T->getAs<PointerType>();
  return PtrType ? PtrType->getPointeeType().getQualifiers() : T->getQualifiers();

================
Comment at: lib/Sema/SemaChecking.cpp:1570-1577
@@ -1550,3 +1569,10 @@
     ValType = AtomTy->getAs<AtomicType>()->getValueType();
+  } else if (Form != Load && Op != AtomicExpr::AO__atomic_load) {
+    if (ValType.isConstQualified()) {
+      Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_pointer)
+        << ValType.getUnqualifiedType() << Ptr->getType()
+        << Ptr->getSourceRange();
+      return ExprError();
+    }
   }
 
----------------
This part LGTM; feel free to factor out this chunk and its tests, and commit it separately.

================
Comment at: lib/Sema/SemaChecking.cpp:1682-1683
@@ +1681,4 @@
+        // as their third argument.
+        if (Op == AtomicExpr::AO__atomic_exchange ||
+            Op == AtomicExpr::AO__atomic_compare_exchange) {
+          Qualifiers ThirdParamQuals = getPointeeQualifiers(*this, TheCall->getArg(2));
----------------
Please don't do this. The first half of this function is carefully classifying the different forms of atomic builtin so that we don't need to reason about the behavior of individual builtins down here.

It'd be better to add a `bool PassInputsByValue = IsC11 || IsN;` in place of the existing `ByValType`, and use either `ValType` or some pointer type built from it depending on the value of `PassInputsByValue`.

================
Comment at: lib/Sema/SemaChecking.cpp:1689
@@ +1688,3 @@
+          if (Op == AtomicExpr::AO__atomic_compare_exchange
+              && ThirdParamQuals.hasConst())
+            Ty.addConst();
----------------
`&&` should go at the end of the line, not the start.

================
Comment at: lib/Sema/SemaChecking.cpp:1692-1693
@@ +1691,4 @@
+          Ty = Context.getPointerType(Ty);
+        }
+        else
+          Ty = ByValType;
----------------
`}` and `else` should go on the same line.

http://reviews.llvm.org/D10407

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/