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

[Eric Fiselier]

Respond to @rsmith's comments.


================
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();
+}
----------------
rsmith wrote:
> 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();
I'll drop the "S.DefaultFunctionArrayLValueConversion" call and use T->getQualifiers() when the type is not a pointer.

================
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();
+    }
   }
 
----------------
rsmith wrote:
> This part LGTM; feel free to factor out this chunk and its tests, and commit it separately.
I'll keep it in this patch. Hopefully I can get this in soon.

================
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));
----------------
rsmith wrote:
> 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`.
Ack. Although your formulation for `PassInputsByValue` is not quite correct. I'll update it with something similar.

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

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

http://reviews.llvm.org/D10407

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