> From: "Christopher Li" <sparse@xxxxxxxxxxx>
> To: "Lance Richardson" <lrichard@xxxxxxxxxx>
> Cc: "Linux-Sparse" <linux-sparse@xxxxxxxxxxxxxxx>
> Sent: Tuesday, 9 May, 2017 11:53:43 AM
> Subject: Re: [PATCH v6] sparse: add support for _Static_assert
>
> Hi Lance,
>
> thanks for the patch. Looks very good.
> Have some very minor comment below:
>
>
> On Mon, May 8, 2017 at 2:15 PM, Lance Richardson <lrichard@xxxxxxxxxx> wrote:
> > @@ -1945,13 +1953,17 @@ static struct token *declaration_list(struct token
> > *token, struct symbol_list **
> > static struct token *struct_declaration_list(struct token *token, struct
> > symbol_list **list)
> > {
> > while (!match_op(token, '}')) {
> > - if (!match_op(token, ';'))
> > - token = declaration_list(token, list);
> > - if (!match_op(token, ';')) {
> > - sparse_error(token->pos, "expected ; at end of
> > declaration");
> > - break;
> > + if (match_ident(token, &_Static_assert_ident))
> > + token = parse_static_assert(token, NULL);
>
> I agree with Luc that his better with a "continue" then
> the following "else" section does not need to changed.
>
>
> > @@ -2093,6 +2105,37 @@ static struct token *parse_asm_declarator(struct
> > token *token, struct decl_state
> > return token;
> > }
> >
> > +static struct token *parse_static_assert(struct token *token, struct
> > symbol_list **unused)
> > +{
> > + struct expression *cond = NULL, *message = NULL;
> > + struct token *errtok;
> > + int valid = 1;
> > +
> > + token = expect(token->next, '(', "after _Static_assert");
> > + errtok = token;
> > + token = constant_expression(token, &cond);
> > + if (!cond) {
>
> I think errtok is not needed here. If there is an expression, the normal
> case. Then errtok will holding the start of the expression. errok->pos
> == cond->pos.
> Call it errtok is a bit miss leading.
>
> If cond == NULL, that means the expression is empty. Then we have
> errtok == token. Either way errtok is not very useful.
>
> > + sparse_error(errtok->pos, "Expected constant expression");
>
> BTW, when the error happen here, "errtok" is actually the next token after
> the missing expression, normally the ','. So the error actually happen before
> that token.
>
OK
> > + valid = 0;
> > + }
> > + token = expect(token, ',', "after conditional expression in
> > _Static_assert");
> > + errtok = token;
> > + token = parse_expression(token, &message);
> > + if (!message || message->type != EXPR_STRING) {
> > + sparse_error(errtok->pos, "bad or missing string literal");
> > + valid = 0;
> > + }
> > + token = expect(token, ')', "after diagnostic message in
> > _Static_assert");
> > +
> > + token = expect(token, ';', "after _Static_assert()");
>
> There is some white space mix with tab in this line.
>
> > +
> > + if (valid && !const_expression_value(cond) && cond->type ==
> > EXPR_VALUE)
>
> I am tempted to get rid of the "valid" variable. BTW, the "cond->type
> == EXPR_VALUE"
> should take place *before* the "!const_expression_value(cond)", other
> wise it will try
> to get const expression for non value expression, due to the evaluate order.
>
> Some thing like:
>
> if (cond && cond->type == EXPR_VALUE &&
> !const_expression_value(cond))
This doesn't work for some cases. E.g. for an expression like
"sizeof(struct s) == 16", cond->type is EXPR_COMPARE before
const_expression_value(cond) is called and is only set to EXPR_VALUE after
the call has reduced the expression to a value. I was looking at this test
as a way to verify that const_expression_value() was successful.
I do think we can get rid of the "valid" variable though, as you suggest.
>
> > + sparse_error(cond->pos, "static assertion failed: %s",
> > + show_string(message->string));
>
> Then there:
> message ? show_string(message->string) : "");
>
> I think the assert failed without message string, we already report
> the error on empty string
> expression. Raising the assert here might be acceptable?
>
I was taking the more conservative approach of not assuming anything
about the interpretation of the two expressions if the assertion is not
syntactically correct. This seems like a better way to go.
> Chris
>
Thanks for the feedback, I will roll a new version shortly.
Lance
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
