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.
> + 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))
> + 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?
Chris
--
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
