Hi Martin,
On Sun, Jan 03, 2016 at 08:03:20PM -0700, Martin Sebor wrote:
> Index: gcc/doc/extend.texi
> ===================================================================
> --- gcc/doc/extend.texi (revision 232047)
> +++ gcc/doc/extend.texi (working copy)
> @@ -9238,6 +9238,8 @@
> @{ tmp = *ptr; *ptr = ~(tmp & value); return tmp; @} // nand
> @end smallexample
>
> +The object pointed to by the first argument must of integer or pointer type.
> It must not be a Boolean type.
Too long line and missing "be " after "must"?
> +The same constraints on arguments apply as for the corresponding
> @code{__sync_op_and_fetch} built-in functions.
> +
Too long line.
> -All memory orders are valid.
> +The object pointed to by the first argument must of integer or pointer type.
> It must not be a Boolean type. All memory orders are valid.
Too long line and missing "be " after "must"?
> +The same constraints on arguments apply as for the corresponding
> @code{__atomic_op_fetch} built-in functions. All memory orders are valid.
Too long line.
> @@ -10686,12 +10691,16 @@
> if (!INTEGRAL_TYPE_P (type) && !POINTER_TYPE_P (type))
> goto incompatible;
>
> + if (fetch && TREE_CODE (type) == BOOLEAN_TYPE)
> + goto incompatible;
This goto is indented two more spaces than it should be.
> @@ -11250,6 +11259,11 @@
> vec<tree, va_gc> *params)
> {
> enum built_in_function orig_code = DECL_FUNCTION_CODE (function);
> +
> + /* Is function is one of the _FETCH_OP_ or _OP_FETCH_ built-ins?
I think drop the second "is".
> @@ -11325,6 +11339,9 @@
> case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_N:
> case BUILT_IN_ATOMIC_LOAD_N:
> case BUILT_IN_ATOMIC_STORE_N:
> + {
> + fetch_op = false;
> + }
Let's either remove those {} or add a fallthrough comment as done above.
> @@ -11358,7 +11375,16 @@
> case BUILT_IN_SYNC_LOCK_TEST_AND_SET_N:
> case BUILT_IN_SYNC_LOCK_RELEASE_N:
> {
> - int n = sync_resolve_size (function, params);
> + /* The following are not _FETCH_OPs and must be accepted with
> + pointers to _Bool (or C++ bool). */
> + if (fetch_op)
> + fetch_op =
> + orig_code != BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_N
> + && orig_code != BUILT_IN_SYNC_VAL_COMPARE_AND_SWAP_N
> + && orig_code != BUILT_IN_SYNC_LOCK_TEST_AND_SET_N
> + && orig_code != BUILT_IN_SYNC_LOCK_RELEASE_N;
> +
Trailing whitespaces on this line. And I think add () around the RHS
of the assignment to fetch_op.
> Index: gcc/testsuite/gcc.dg/atomic-fetch-bool.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/atomic-fetch-bool.c (revision 0)
> +++ gcc/testsuite/gcc.dg/atomic-fetch-bool.c (working copy)
> @@ -0,0 +1,64 @@
> +/* PR c/68966 - atomic_fetch_* on atomic_bool not diagnosed
> + Test to verify that calls to __atomic_fetch_op funcions with a _Bool
> + argument are rejected. This is necessary because GCC expects that
> + all initialized _Bool objects have a specific representation and
> + allowing atomic operations to change it would break the invariant. */
> +/* { dg-do compile } */
> +/* { dg-options "-std=c11" } */
Doesn't matter here, but probably add -pedantic-errors.
> Index: gcc/testsuite/gcc.dg/sync-fetch-bool.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/sync-fetch-bool.c (revision 0)
> +++ gcc/testsuite/gcc.dg/sync-fetch-bool.c (working copy)
> @@ -0,0 +1,54 @@
> +/* PR c/68966 - atomic_fetch_* on atomic_bool not diagnosed
> + Test to verify that calls to __sync_fetch_op funcions with a _Bool
> + argument are rejected. This is necessary because GCC expects that
> + all initialized _Bool objects have a specific representation and
> + allowing atomic operations to change it would break the invariant. */
> +/* { dg-do compile } */
> +/* { dg-options "-std=c99" } */
As the testcase uses _Atomic, I wonder why there's -std=c99. I'd use
-std=c11 -pedantic-errors.
Thanks,
Marek
