Re: [PATCH] sparse: add support for static assert

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Sun, Jan 10, 2016 at 09:54:24PM -0500, Lance Richardson wrote:
> ----- Original Message -----
> > On Wed, Jan 06, 2016 at 01:35:17PM -0500, Lance Richardson wrote:
> > > This patch introduces support for _Static_assert() in global,
> > > function, and struct/union declaration contexts (as currently supported
> > > by gcc).
> > > 
> > > Tested via:
> > >    - kernel build with C=1 CF=-D__CHECK_ENDIAN__
> > >    - build/check large code base making heavy use of _Static_assert()
> > >    - "make check" with added test case for static assert support
> > 
> > Nice, it was something that was indeed missing.
> > 
> > I have a few remarks, mostly nitpicking, here under.
> 
> Hi Luc, thanks for taking the time to review this.

Glad to help!
 
...

> > 
> > I find also a bit strange to have this sort of semantic check inside the
> > parser.
> 
> This prompted me to look at the gcc implementation, it appears that gcc also
> processes _Static_assert() during parsing.

It can always be moved elsewhere if needed.
 

> > Also, what should happen with:
> > 	_Static_assert(1 / 0, "invalid expression: should not show up?");
> > (The C11 standard is not clear to me, GCC outputs an "invalid expression"
> > message and ignore the assertion).
> 
> v2 will output something like "invalid constant integer expression".

Please don't.
That's the job of constant_expression() to emit a good error message.
Here, the best thing to do is to *not* emit an error message if the
expression is not a valid expression, constant (for example,
by forcing val to 1 when expr is null).

> > 
> > Finally, the standard also allow to place the assertion
> > inside a struct or union declaration; like:
> > 	struct s {
> > 		char c;
> > 		_Static_assert(1, "inside struct");
> > 	};
> > which is working fine.
> > And also, I think:
> > 	struct s2 {
> > 		char c;
> > 		_Static_assert(sizeof(struct s) == 1, "own struct sizeof");
> > 	};
> > which doesn't.
> 
> The second case should work (unless you intended "sizeof(struct s2)"), the v2
> implementation will be a little different to address this issue.

Yep, I messed it up.
I indeed meant "sizeof(struct s2)" and it seems to work fine
but for the wrong reason. See what I mean with the following example:
	struct s3 {
	        char c;
	        _Static_assert(sizeof(struct s3) == 1, "own struct sizeof, partial 1");
	        _Static_assert(sizeof(struct s3) == 2, "own struct sizeof, partial 2");
	        char d;
	};
but this problem is unrelated to your patch.

Yours,
Luc