Chris Li wrote:
> On Sat, May 25, 2013 at 09:01:30PM +0100, Ramsay Jones wrote:
>> +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".
OK, see below.
>
>>
>> + /* 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.
OK
>
>> 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.
Heh, indeed.
>
> If you have better way to write it I am open to it.
>
Well, I don't know that it's better; but the following patch (with or
without the diff applied) *reads* better to me. :-D
Note that the patch given below, after the scissors mark, keeps the
"fall through" into the string case, but I actually prefer not to do
so, and would add this (at least) on top:
diff --git a/symbol.c b/symbol.c
index 4e7b6e2..06e9596 100644
--- a/symbol.c
+++ b/symbol.c
@@ -312,12 +312,12 @@ static int count_array_initializer(struct symbol *t, struct expression *expr)
case EXPR_PREOP:
/* check for: char x[] = {("string")}; */
p = parenthesized_string(entry);
- if (!(is_char && p)) {
- nr++;
- break;
+ if (is_char && p) {
+ entry = p;
+ str_len = entry->string->length;
}
- entry = p;
- /* fall through to string */
+ nr++;
+ break;
case EXPR_STRING:
if (is_char)
str_len = entry->string->length;
@@ -332,10 +332,11 @@ static int count_array_initializer(struct symbol *t, struct expression *expr)
case EXPR_PREOP:
/* check for parenthesized string: char x[] = ("string"); */
p = parenthesized_string(expr);
- if (!(is_char && p))
- break;
- expr = p;
- /* fall through to string */
+ if (is_char && p) {
+ expr = p;
+ nr = expr->string->length;
+ }
+ break;
case EXPR_STRING:
if (is_char)
nr = expr->string->length;
Actually, I've just noticed that the last hunk above can be simplified
even further (there is no need to assign p to expr, just use p directly
in the assignment to nr).
[I would also consider making the string case self contained and not
falling through to the default case; i.e. add the "nr++;" and "break;"
statements.]
I have tested the patch below as-is, *and* with the above squashed in, on
Linux, cygwin and MinGW. Note, also, that the test has been fixed up.
Anyway, I will leave to choice of solution to you. ;-)
HTH
ATB,
Ramsay Jones
-- >8 --
From: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxx>
Date: Thu, 16 May 2013 20:44:11 +0100
Subject: [PATCH] symbol.c: Set correct size of array from parenthesized string initializer
Signed-off-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Christopher Li <sparse@xxxxxxxxxxx>
---
symbol.c | 30 ++++++++++++++++++++++++++++++
validation/init-char-array1.c | 28 ++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
create mode 100644 validation/init-char-array1.c
diff --git a/symbol.c b/symbol.c
index 80a2f23..4e7b6e2 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 *parenthesized_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;
@@ -296,6 +309,15 @@ 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")}; */
+ p = parenthesized_string(entry);
+ if (!(is_char && p)) {
+ nr++;
+ break;
+ }
+ entry = p;
+ /* fall through to string */
case EXPR_STRING:
if (is_char)
str_len = entry->string->length;
@@ -307,9 +329,17 @@ static int count_array_initializer(struct symbol *t, struct expression *expr)
nr = str_len;
break;
}
+ case EXPR_PREOP:
+ /* check for parenthesized string: char x[] = ("string"); */
+ p = parenthesized_string(expr);
+ if (!(is_char && p))
+ break;
+ expr = p;
+ /* fall through to string */
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..923741f
--- /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 w[] = {("hello")};
+static const char x[] = "hello";
+static const char y[5] = "hello";
+
+static void f(void)
+{
+ char a[1/(sizeof(u) == 6)];
+ char b[1/(sizeof(v) == 6)];
+ char c[1/(sizeof(w) == 6)];
+ char d[1/(sizeof(x) == 6)];
+ char e[1/(sizeof(y) == 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:27: warning: array initialized from parenthesized string constant
+ * check-error-end
+ */
--
1.8.3
--
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
