================
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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
