Re: Invalid _Static_assert expanded from HASH_CALLBACKS_CHECK

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2021 06:29, Roberto Bagnara wrote:
> I stumbled upon parsing errors due to invalid uses of
> _Static_assert expanded from HASH_CALLBACKS_CHECK where
> the tested expression is not constant, as mandated by
> the C standard.
> 
> Judging from the following comment, there is partial awareness
> of the fact this is an issue:
> 
> #ifndef __clang__ /* At least some versions dislike some of the uses. */
> #define HASH_CALLBACKS_CHECK(mask) \
>      BUILD_BUG_ON((mask) > (1U << ARRAY_SIZE(callbacks)) - 1)
> 
> Indeed, this is not a fault of Clang: the point is that some
> of the expansions of this macro are not C.  Moreover,
> the fact that GCC sometimes accepts them is not
> something we can rely upon:
> 
> $ cat p.c
> void f() {
> static const int x = 3;
> _Static_assert(x < 4, "");
> }
> $ gcc -c -O p.c
> $ gcc -c p.c
> p.c: In function ‘f’:
> p.c:3:20: error: expression in static assertion is not constant
> 3 | _Static_assert(x < 4, "");
> | ~^~
> $

I'd nevertheless like to stick to this as long as not proven
otherwise by future gcc.

> Finally, I think this can be easily avoided: instead
> of initializing a static const with a constant expression
> and then static-asserting the static const, just static-assert
> the constant initializer.

Well, yes, but the whole point of constructs like

    HASH_CALLBACKS_CHECK(callback_mask);
    hash_domain_foreach(d, callback_mask, callbacks, gmfn);

is to make very obvious that the checked mask and the used mask
match. Hence if anything I'd see us eliminate the static const
callback_mask variables altogether. I did avoid doing so in the
earlier change, following the assumption that the choice of
using a static const there was for a reason originally (my guess:
a combination of not wanting to use a #define and of having the
mask values live next to their corresponding arrays).

Cc-ing Tim as the maintainer, to possibly override my views.

Jan