yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:6576-6578
if (!ValType->isFloatingType())
return false;
+ if (!(AllowedType & AOAVT_FP))
----------------
tra wrote:
> Collapse into a single if statement: `if (!(ValType->isFloatingType() &&
> (AllowedType & AOAVT_FP)))`
will do
================
Comment at: clang/lib/Sema/SemaChecking.cpp:6588
+ if (!IsAllowedValueType(ValType, ArithAllows)) {
+ assert(ArithAllows & AOAVT_Integer);
+ auto DID = ArithAllows & AOAVT_FP
----------------
tra wrote:
> Why do we expect a failed `IsAllowedValueType` check to fail only if we were
> allowed integers? Is that because we assume that all atomic instructions
> support integers?
>
> If that's the case, I'd hoist the assertion and apply it right after we're
> done setting `ArithAllows`. Alternatively, we could discard `AOAVT_Integer`
> and call the enum `ArithOpExtraValueType`. Tracking a bit that's always set
> does not buy us much, though it does make the code a bit more uniform. Up to
> you.
>
>
>
we assume integer is always allowed. AOAVT_Integer is for uniformity. I will
change the enum to ArithOpExtraValueType and remove AOAVT_Integer
================
Comment at: clang/test/Sema/atomic-ops.c:134
int *I, const int *CI,
int **P, float *D, struct S *s1, struct S *s2) {
__c11_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
----------------
tra wrote:
> I wonder why we have this inconsistency in the non-atomic arguments.
> We don't actually have any double variants and the argument `D` is actually a
> `float *`, even though the naming convention used suggests that it should've
> been either a `double *` or should be called `F`.
>
will rename it to F and add double *D
================
Comment at: clang/test/Sema/atomic-ops.c:218
__atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be
a pointer to integer, pointer or supported floating point type}}
- __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be
a pointer to integer}}
- __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be
a pointer to integer}}
+ __atomic_fetch_min(D, 3, memory_order_seq_cst);
+ __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be
a pointer to integer or supported floating point type}}
----------------
tra wrote:
> We seem to be missing the tests for `double *` here, too.
will add it
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150985/new/
https://reviews.llvm.org/D150985
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
