| Message ID | 51A1189A.1010404@ramsay1.demon.co.uk (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Sat, May 25, 2013 at 09:01:30PM +0100, Ramsay Jones wrote: > Yes, this patch works. > > However, it made me go cross-eyed trying to follow the flow of > control. So, I created a new patch, added after the scissor mark > below, which hopefully makes the flow of control easier to follow. As my patch, you should just read the "case EXPR_STRING" as a label, that is easier to understand. Unfortunately I think your patch is not doing the same thing as mine, control flow wise. Please see the later comment on your code. > Also, note that I changed the test to add a check for the length > of the v1 array. Having said that, I had intended to rename the > variables in the test to u->y and a->e (this is a new test, after > all), but I forgot! sorry about that. Can you submit a separate delta for the test part of the change? I can smash the patch on top your original one. > +static struct expression *paren_string(struct expression *expr) Paren sounds very much like "parent". I would make the function name "parenthesized_string" or "extract_parenthesized_string". > > + /* check for a parenthesized string: char x[] = ("string"); */ > + if (is_char && (p = paren_string(expr))) > + expr = p; > + I want to move this test to inside the switch statement. The parenthesized string is a very rare case. Adding the test here make the common case pay a price. > switch (expr->type) { > case EXPR_INITIALIZER: { > struct expression *entry; > @@ -296,6 +313,10 @@ static int count_array_initializer(struct symbol *t, struct expression *expr) > if (entry->idx_to >= nr) > nr = entry->idx_to+1; > break; > + case EXPR_PREOP: > + /* check for char x[] = {("string")}; */ > + if (is_char && (p = paren_string(entry))) > + entry = p; > case EXPR_STRING: > if (is_char) > str_len = entry->string->length; OK, here is the subtle bug. Consider the case expr->type == EXPR_PREOP and is_char == 1 and paren_string(entry) return false. Before apply your patch, it will go to default case. Now after your patch, it will go to execute "str_len = entry->string->length", even entry expression is not a string. So your patch is doing some thing not intended. In order to correct that, you can't always allow EXPR_PREOP case fall through to EXPR_STRING. It need to depend on the test condition paren_string() return true or not. You can add goto statement to the switch statement to fix that. It will make the control flow hard to read as well. If you have better way to write it I am open to it. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/symbol.c b/symbol.c index 80a2f23..55e1b47 100644 --- a/symbol.c +++ b/symbol.c @@ -269,8 +269,21 @@ void merge_type(struct symbol *sym, struct symbol *base_type) merge_type(sym, sym->ctype.base_type); } +static struct expression *paren_string(struct expression *expr) +{ + if (expr && expr->type == EXPR_PREOP && expr->op == '(') { + struct expression *e = expr; + while (e && e->type == EXPR_PREOP && e->op == '(') + e = e->unop; + if (e && e->type == EXPR_STRING) + return e; + } + return NULL; +} + static int count_array_initializer(struct symbol *t, struct expression *expr) { + struct expression *p; int nr = 0; int is_char = 0; @@ -284,6 +297,10 @@ static int count_array_initializer(struct symbol *t, struct expression *expr) if (t->ctype.base_type == &int_type && t->ctype.modifiers & MOD_CHAR) is_char = 1; + /* check for a parenthesized string: char x[] = ("string"); */ + if (is_char && (p = paren_string(expr))) + expr = p; + switch (expr->type) { case EXPR_INITIALIZER: { struct expression *entry; @@ -296,6 +313,10 @@ static int count_array_initializer(struct symbol *t, struct expression *expr) if (entry->idx_to >= nr) nr = entry->idx_to+1; break; + case EXPR_PREOP: + /* check for char x[] = {("string")}; */ + if (is_char && (p = paren_string(entry))) + entry = p; case EXPR_STRING: if (is_char) str_len = entry->string->length; @@ -310,6 +331,7 @@ static int count_array_initializer(struct symbol *t, struct expression *expr) case EXPR_STRING: if (is_char) nr = expr->string->length; + break; default: break; } diff --git a/validation/init-char-array1.c b/validation/init-char-array1.c new file mode 100644 index 0000000..bd4ed68 --- /dev/null +++ b/validation/init-char-array1.c @@ -0,0 +1,28 @@ +/* + * for array of char, ("...") as the initializer is an gcc language + * extension. check that a parenthesized string initializer is handled + * correctly and that -Wparen-string warns about it's use. + */ +static const char u[] = ("hello"); +static const char v[] = {"hello"}; +static const char v1[] = {("hello")}; +static const char w[] = "hello"; +static const char x[5] = "hello"; + +static void f(void) +{ + char a[1/(sizeof(u) == 6)]; + char b[1/(sizeof(v) == 6)]; + char b1[1/(sizeof(v1) == 6)]; + char c[1/(sizeof(w) == 6)]; + char d[1/(sizeof(x) == 5)]; +} +/* + * check-name: parenthesized string initializer + * check-command: sparse -Wparen-string $file + * + * check-error-start +init-char-array1.c:6:26: warning: array initialized from parenthesized string constant +init-char-array1.c:8:28: warning: array initialized from parenthesized string constant + * check-error-end + */