* [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings
@ 2020-05-18 23:54 Luc Van Oostenryck
2020-05-20 0:22 ` Ramsay Jones
0 siblings, 1 reply; 6+ messages in thread
From: Luc Van Oostenryck @ 2020-05-18 23:54 UTC (permalink / raw)
To: linux-sparse
Cc: Ramsay Jones, Junio C Hamano,
Đoàn Trần Công Danh, Luc Van Oostenryck
In standard C '{ 0 }' is valid to initialize any compound object.
OTOH, Sparse allows '{ }' for the same purpose but:
1) '{ }' is not standard
2) Sparse warns when using '0' to initialize pointers.
Some projects (git) legitimately like to be able to use the
standard '{ 0 }' without the null-pointer warnings
So, add a new warning flag (-Wno-universal-initializer) to
handle '{ 0 }' as '{ }', suppressing the warnings.
Reference: https://lore.kernel.org/git/1df91aa4-dda5-64da-6ae3-5d65e50a55c5@ramsayjones.plus.com/
Reference: https://lore.kernel.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
Suggestions for a better name than this -W[no-]universal-initializer
are warmly welcome.
-- Luc
lib.c | 2 ++
lib.h | 1 +
parse.c | 7 +++++++
sparse.1 | 8 ++++++++
token.h | 7 +++++++
validation/Wuniv-init-ko.c | 14 ++++++++++++++
validation/Wuniv-init-ok.c | 11 +++++++++++
7 files changed, 50 insertions(+)
create mode 100644 validation/Wuniv-init-ko.c
create mode 100644 validation/Wuniv-init-ok.c
diff --git a/lib.c b/lib.c
index f9ec285e8fea..9ee8d3cf6b21 100644
--- a/lib.c
+++ b/lib.c
@@ -295,6 +295,7 @@ int Wtransparent_union = 0;
int Wtypesign = 0;
int Wundef = 0;
int Wuninitialized = 1;
+int Wuniversal_initializer = 1;
int Wunknown_attribute = 0;
int Wvla = 1;
@@ -782,6 +783,7 @@ static const struct flag warnings[] = {
{ "typesign", &Wtypesign },
{ "undef", &Wundef },
{ "uninitialized", &Wuninitialized },
+ { "universal-initializer", &Wuniversal_initializer },
{ "unknown-attribute", &Wunknown_attribute },
{ "vla", &Wvla },
};
diff --git a/lib.h b/lib.h
index b18295a889cb..5e6db111170a 100644
--- a/lib.h
+++ b/lib.h
@@ -184,6 +184,7 @@ extern int Wtransparent_union;
extern int Wtypesign;
extern int Wundef;
extern int Wuninitialized;
+extern int Wuniversal_initializer;
extern int Wunknown_attribute;
extern int Wvla;
diff --git a/parse.c b/parse.c
index a29c67c8cf41..48494afc6f2c 100644
--- a/parse.c
+++ b/parse.c
@@ -2750,6 +2750,13 @@ static struct token *initializer_list(struct expression_list **list, struct toke
{
struct expression *expr;
+ // '{ 0 }' is equivalent to '{ }' unless wanting all possible
+ // warnings about using '0' to initialize a null-pointer.
+ if (!Wuniversal_initializer) {
+ if (match_token_zero(token) && match_op(token->next, '}'))
+ token = token->next;
+ }
+
for (;;) {
token = single_initializer(&expr, token);
if (!expr)
diff --git a/sparse.1 b/sparse.1
index 574caef3acbb..50e928392573 100644
--- a/sparse.1
+++ b/sparse.1
@@ -428,6 +428,14 @@ However, this behavior can lead to subtle errors.
Sparse does not issue these warnings by default.
.
+.TP
+.B \-Wuniversal\-initializer
+Do not suppress warnings about 0 used to initialize a null-pointer
+when using '{ 0 }' as initializer.
+
+Sparse issues these warnings by default. To turn them off, use
+\fB\-Wno\-universal\-initializer\fR.
+.
.SH MISC OPTIONS
.TP
.B \-\-arch=\fIARCH\fR
diff --git a/token.h b/token.h
index 292db167e4a8..33a6eda1cc53 100644
--- a/token.h
+++ b/token.h
@@ -241,4 +241,11 @@ static inline int match_ident(struct token *token, struct ident *id)
return token->pos.type == TOKEN_IDENT && token->ident == id;
}
+static inline int match_token_zero(struct token *token)
+{
+ if (token_type(token) != TOKEN_NUMBER)
+ return false;
+ return token->number[0] == '0' && !token->number[1];
+}
+
#endif
diff --git a/validation/Wuniv-init-ko.c b/validation/Wuniv-init-ko.c
new file mode 100644
index 000000000000..315c211a5db6
--- /dev/null
+++ b/validation/Wuniv-init-ko.c
@@ -0,0 +1,14 @@
+struct s {
+ void *ptr;
+};
+
+
+static struct s s = { 0 };
+
+/*
+ * check-name: univ-init-ko
+ *
+ * check-error-start
+Wuniv-init-ko.c:6:23: warning: Using plain integer as NULL pointer
+ * check-error-end
+ */
diff --git a/validation/Wuniv-init-ok.c b/validation/Wuniv-init-ok.c
new file mode 100644
index 000000000000..c39647517323
--- /dev/null
+++ b/validation/Wuniv-init-ok.c
@@ -0,0 +1,11 @@
+struct s {
+ void *ptr;
+};
+
+
+static struct s s = { 0 };
+
+/*
+ * check-name: univ-init-ok
+ * check-command: sparse -Wno-universal-initializer $file
+ */
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings
2020-05-18 23:54 [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings Luc Van Oostenryck
@ 2020-05-20 0:22 ` Ramsay Jones
2020-05-20 0:41 ` Đoàn Trần Công Danh
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ramsay Jones @ 2020-05-20 0:22 UTC (permalink / raw)
To: Luc Van Oostenryck, linux-sparse
Cc: Junio C Hamano, Đoàn Trần Công Danh
Hi Luc,
Sorry for not getting back to you sooner on this (you would
think I was busy! ;-D ).
I have now found (one of) the patch(es) I was referring to before
(as a patch file on a memory stick - don't ask!).
On 19/05/2020 00:54, Luc Van Oostenryck wrote:
> In standard C '{ 0 }' is valid to initialize any compound object.
> OTOH, Sparse allows '{ }' for the same purpose but:
> 1) '{ }' is not standard
> 2) Sparse warns when using '0' to initialize pointers.
>
> Some projects (git) legitimately like to be able to use the
> standard '{ 0 }' without the null-pointer warnings
>
> So, add a new warning flag (-Wno-universal-initializer) to
> handle '{ 0 }' as '{ }', suppressing the warnings.
Hmm, I didn't think this would use a warning flag at all!
I remember the discussion (on lkml and sparse ml) in which
there was general agreement that '{}' would be preferred
solution (if only it was standard C!). However, I thought
that (since some compilers don't support it e.g. msvc) the
next best solution would be for sparse to suppress the
warning if given the '= { 0 }' token sequence. (ie. no mention
of it being conditional on a option).
>
> Reference: https://lore.kernel.org/git/1df91aa4-dda5-64da-6ae3-5d65e50a55c5@ramsayjones.plus.com/
> Reference: https://lore.kernel.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>
> Suggestions for a better name than this -W[no-]universal-initializer
> are warmly welcome.
Heh, you know that I am no good at naming things - but this may well
give the impression of a C++ like 'int i{}' type initializer!
>
> -- Luc
>
>
> lib.c | 2 ++
> lib.h | 1 +
> parse.c | 7 +++++++
> sparse.1 | 8 ++++++++
> token.h | 7 +++++++
> validation/Wuniv-init-ko.c | 14 ++++++++++++++
> validation/Wuniv-init-ok.c | 11 +++++++++++
> 7 files changed, 50 insertions(+)
> create mode 100644 validation/Wuniv-init-ko.c
> create mode 100644 validation/Wuniv-init-ok.c
>
> diff --git a/lib.c b/lib.c
> index f9ec285e8fea..9ee8d3cf6b21 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -295,6 +295,7 @@ int Wtransparent_union = 0;
> int Wtypesign = 0;
> int Wundef = 0;
> int Wuninitialized = 1;
> +int Wuniversal_initializer = 1;
> int Wunknown_attribute = 0;
> int Wvla = 1;
>
> @@ -782,6 +783,7 @@ static const struct flag warnings[] = {
> { "typesign", &Wtypesign },
> { "undef", &Wundef },
> { "uninitialized", &Wuninitialized },
> + { "universal-initializer", &Wuniversal_initializer },
> { "unknown-attribute", &Wunknown_attribute },
> { "vla", &Wvla },
> };
> diff --git a/lib.h b/lib.h
> index b18295a889cb..5e6db111170a 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -184,6 +184,7 @@ extern int Wtransparent_union;
> extern int Wtypesign;
> extern int Wundef;
> extern int Wuninitialized;
> +extern int Wuniversal_initializer;
> extern int Wunknown_attribute;
> extern int Wvla;
>
> diff --git a/parse.c b/parse.c
> index a29c67c8cf41..48494afc6f2c 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -2750,6 +2750,13 @@ static struct token *initializer_list(struct expression_list **list, struct toke
> {
> struct expression *expr;
>
> + // '{ 0 }' is equivalent to '{ }' unless wanting all possible
> + // warnings about using '0' to initialize a null-pointer.
> + if (!Wuniversal_initializer) {
> + if (match_token_zero(token) && match_op(token->next, '}'))
> + token = token->next;
> + }
> +
Ha! This made me LOL! (see my patch below).
So simple. (I did think, at first, that deleting the '0' token was
not a good idea - then I realized that it's more like skipping/ignoring
the token than deleting it.)
I wish I had thought of it.
I have similar code in 'initializer()', rather than 'initializer_list()',
but I don't think it makes a big difference (you have already 'passed'
the initial '{' token).
> for (;;) {
> token = single_initializer(&expr, token);
> if (!expr)
> diff --git a/sparse.1 b/sparse.1
> index 574caef3acbb..50e928392573 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -428,6 +428,14 @@ However, this behavior can lead to subtle errors.
>
> Sparse does not issue these warnings by default.
> .
> +.TP
> +.B \-Wuniversal\-initializer
> +Do not suppress warnings about 0 used to initialize a null-pointer
> +when using '{ 0 }' as initializer.
> +
> +Sparse issues these warnings by default. To turn them off, use
> +\fB\-Wno\-universal\-initializer\fR.
> +.
> .SH MISC OPTIONS
> .TP
> .B \-\-arch=\fIARCH\fR
> diff --git a/token.h b/token.h
> index 292db167e4a8..33a6eda1cc53 100644
> --- a/token.h
> +++ b/token.h
> @@ -241,4 +241,11 @@ static inline int match_ident(struct token *token, struct ident *id)
> return token->pos.type == TOKEN_IDENT && token->ident == id;
> }
>
> +static inline int match_token_zero(struct token *token)
> +{
> + if (token_type(token) != TOKEN_NUMBER)
> + return false;
> + return token->number[0] == '0' && !token->number[1];
> +}
> +
> #endif
> diff --git a/validation/Wuniv-init-ko.c b/validation/Wuniv-init-ko.c
> new file mode 100644
> index 000000000000..315c211a5db6
> --- /dev/null
> +++ b/validation/Wuniv-init-ko.c
> @@ -0,0 +1,14 @@
> +struct s {
> + void *ptr;
> +};
> +
> +
> +static struct s s = { 0 };
> +
> +/*
> + * check-name: univ-init-ko
> + *
> + * check-error-start
> +Wuniv-init-ko.c:6:23: warning: Using plain integer as NULL pointer
> + * check-error-end
> + */
> diff --git a/validation/Wuniv-init-ok.c b/validation/Wuniv-init-ok.c
> new file mode 100644
> index 000000000000..c39647517323
> --- /dev/null
> +++ b/validation/Wuniv-init-ok.c
> @@ -0,0 +1,11 @@
> +struct s {
> + void *ptr;
> +};
> +
> +
> +static struct s s = { 0 };
> +
> +/*
> + * check-name: univ-init-ok
> + * check-command: sparse -Wno-universal-initializer $file
> + */
>
The patch below was (I think) my third attempt. If memory serves
me, the first patch attempted to determine the '{0}' initializer
from the 'struct expession *' passed to bad_null() alone. However,
that did not allow me to distinguish '= { 0 }' from '= { 0, }',
so I needed to backup from evaluation to the parse.
Also, I remember that I wasn't happy with the test cases - this code
(should) affect the initialization of arrays of pointers, but I have
not even written any tests, let alone tried them! :(
Also, I didn't test the initialization of embedded struct/array fields
(and what should happen anyway? should '{ 0 }' also work for initializing
the sub-structure(s), or should it only work at the top-level?).
Also, I have just noticed, it seems that I can't decide if it should
be called 'zero aggregate initializer' or 'aggregate zero initializer'! :-P
ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] bad_null: suppress 0/NULL pointer warnings with '{0}'
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
evaluate.c | 3 ++-
expression.h | 1 +
parse.c | 30 +++++++++++++++++++++++
validation/aggregate_zero_init.c | 42 ++++++++++++++++++++++++++++++++
4 files changed, 75 insertions(+), 1 deletion(-)
create mode 100755 validation/aggregate_zero_init.c
diff --git a/evaluate.c b/evaluate.c
index b7bb1f52..a95e157e 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -820,8 +820,9 @@ const char *type_difference(struct ctype *c1, struct ctype *c2,
static void bad_null(struct expression *expr)
{
- if (Wnon_pointer_null)
+ if (Wnon_pointer_null && !expr->zero_init) {
warning(expr->pos, "Using plain integer as NULL pointer");
+ }
}
static unsigned long target_qualifiers(struct symbol *type)
diff --git a/expression.h b/expression.h
index 3b79e0f1..4bcfe0aa 100644
--- a/expression.h
+++ b/expression.h
@@ -150,6 +150,7 @@ struct asm_operand {
struct expression {
enum expression_type type:8;
unsigned flags:8;
+ unsigned zero_init:1;
int op;
struct position pos;
struct symbol *ctype;
diff --git a/parse.c b/parse.c
index a29c67c8..44aea888 100644
--- a/parse.c
+++ b/parse.c
@@ -2762,12 +2762,42 @@ static struct token *initializer_list(struct expression_list **list, struct toke
return token;
}
+static int is_zero_token(struct token *token)
+{
+ return token_type(token) == TOKEN_NUMBER && !strcmp(token->number, "0");
+}
+
+/*
+ * starting with token, do we have an '{ 0 }' zero aggregate initializer
+ * token sequence?
+*/
+static int is_zero_aggregate_init(struct token *token)
+{
+ if (token && match_op(token, '{') &&
+ token->next && is_zero_token(token->next) &&
+ token->next->next && match_op(token->next->next, '}'))
+ return 1;
+ return 0;
+}
+
+static void set_zero_init(struct expression_list *list)
+{
+ if (expression_list_size(list) == 1) {
+ struct expression *expr = first_expression(list);
+ if (expr)
+ expr->zero_init = 1;
+ }
+}
+
struct token *initializer(struct expression **tree, struct token *token)
{
if (match_op(token, '{')) {
struct expression *expr = alloc_expression(token->pos, EXPR_INITIALIZER);
+ int zero_init = is_zero_aggregate_init(token);
*tree = expr;
token = initializer_list(&expr->expr_list, token->next);
+ if (zero_init)
+ set_zero_init(expr->expr_list);
return expect(token, '}', "at end of initializer");
}
return assignment_expression(token, tree);
diff --git a/validation/aggregate_zero_init.c b/validation/aggregate_zero_init.c
new file mode 100755
index 00000000..805adbfa
--- /dev/null
+++ b/validation/aggregate_zero_init.c
@@ -0,0 +1,42 @@
+#define NULL ((void *)0)
+
+struct ptrfirst {
+ char *ptr;
+ int scalar;
+};
+
+struct scalarfirst {
+ int scalar;
+ char *ptr;
+};
+
+static struct ptrfirst s1;
+static struct scalarfirst s2;
+
+static struct ptrfirst si1 = { 0 };
+static struct scalarfirst si2 = { 0 };
+
+static struct ptrfirst si3 = { 0, 0 };
+static struct scalarfirst si4 = { 0, 0 };
+
+static struct ptrfirst si5 = { NULL, 0 };
+static struct scalarfirst si6 = { 0, NULL };
+
+static struct ptrfirst si7 = { 0, };
+static struct scalarfirst si8 = { 0, };
+
+static void func(void)
+{
+ struct ptrfirst a1 = { 0 };
+ struct scalarfirst a1 = { 0 };
+}
+/*
+ * check-name: zero aggregate initializer suppresses NULL pointer warnings
+ *
+ * check-error-start
+aggregate_zero_init.c:19:32: warning: Using plain integer as NULL pointer
+aggregate_zero_init.c:20:38: warning: Using plain integer as NULL pointer
+aggregate_zero_init.c:25:32: warning: Using plain integer as NULL pointer
+ * check-error-end
+ */
+
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings
2020-05-20 0:22 ` Ramsay Jones
@ 2020-05-20 0:41 ` Đoàn Trần Công Danh
2020-05-20 20:40 ` Luc Van Oostenryck
2020-06-02 16:41 ` Luc Van Oostenryck
2 siblings, 0 replies; 6+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-20 0:41 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Luc Van Oostenryck, linux-sparse, Junio C Hamano
Hi Luc,
On 2020-05-20 01:22:22+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> > In standard C '{ 0 }' is valid to initialize any compound object.
> > OTOH, Sparse allows '{ }' for the same purpose but:
> > 1) '{ }' is not standard
> > 2) Sparse warns when using '0' to initialize pointers.
> >
> > Some projects (git) legitimately like to be able to use the
> > standard '{ 0 }' without the null-pointer warnings
> >
> > So, add a new warning flag (-Wno-universal-initializer) to
> > handle '{ 0 }' as '{ }', suppressing the warnings.
>
> Hmm, I didn't think this would use a warning flag at all!
>
> I remember the discussion (on lkml and sparse ml) in which
> there was general agreement that '{}' would be preferred
> solution (if only it was standard C!). However, I thought
> that (since some compilers don't support it e.g. msvc) the
> next best solution would be for sparse to suppress the
> warning if given the '= { 0 }' token sequence. (ie. no mention
> of it being conditional on a option).
I'm also in the camp of favouring no -W at all.
But, have another -W is fine to me.
> > Suggestions for a better name than this -W[no-]universal-initializer
> > are warmly welcome.
>
> Heh, you know that I am no good at naming things - but this may well
> give the impression of a C++ like 'int i{}' type initializer!
From this discussion in GCC's BugZilla [1], I think compiler people
tend to call that style as zero-initialization, or universal zero
initialization.
[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119#c12
--
Danh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings
2020-05-20 0:22 ` Ramsay Jones
2020-05-20 0:41 ` Đoàn Trần Công Danh
@ 2020-05-20 20:40 ` Luc Van Oostenryck
2020-05-20 22:03 ` Ramsay Jones
2020-06-02 16:41 ` Luc Van Oostenryck
2 siblings, 1 reply; 6+ messages in thread
From: Luc Van Oostenryck @ 2020-05-20 20:40 UTC (permalink / raw)
To: Ramsay Jones
Cc: linux-sparse, Junio C Hamano, Đoàn Trần Công Danh
On Wed, May 20, 2020 at 01:22:22AM +0100, Ramsay Jones wrote:
> Hi Luc,
>
> Sorry for not getting back to you sooner on this (you would
> think I was busy! ;-D ).
No problem, really. And I haven't been quite reactive myself lately.
> I have now found (one of) the patch(es) I was referring to before
> (as a patch file on a memory stick - don't ask!).
I won't, promise ;)
> On 19/05/2020 00:54, Luc Van Oostenryck wrote:
> > In standard C '{ 0 }' is valid to initialize any compound object.
> > OTOH, Sparse allows '{ }' for the same purpose but:
> > 1) '{ }' is not standard
> > 2) Sparse warns when using '0' to initialize pointers.
> >
> > Some projects (git) legitimately like to be able to use the
> > standard '{ 0 }' without the null-pointer warnings
> >
> > So, add a new warning flag (-Wno-universal-initializer) to
> > handle '{ 0 }' as '{ }', suppressing the warnings.
>
> Hmm, I didn't think this would use a warning flag at all!
>
> I remember the discussion (on lkml and sparse ml) in which
> there was general agreement that '{}' would be preferred
> solution (if only it was standard C!). However, I thought
> that (since some compilers don't support it e.g. msvc) the
> next best solution would be for sparse to suppress the
> warning if given the '= { 0 }' token sequence. (ie. no mention
> of it being conditional on a option).
Yes, I kinda agree but concerning the kernel, my understanding is
that the warning is desired (cfr. https://marc.info/?t=154704602900003 )
For example, for cases like:
int *array[16] = { 0 };
So, I want to keep the current behavior as the default.
> ... but this may well
> give the impression of a C++ like 'int i{}' type initializer!
This syntax is really terrible **shiver**.
> > @@ -2750,6 +2750,13 @@ static struct token *initializer_list(struct expression_list **list, struct toke
> > {
> > struct expression *expr;
> >
> > + // '{ 0 }' is equivalent to '{ }' unless wanting all possible
> > + // warnings about using '0' to initialize a null-pointer.
> > + if (!Wuniversal_initializer) {
> > + if (match_token_zero(token) && match_op(token->next, '}'))
> > + token = token->next;
> > + }
> > +
>
> Ha! This made me LOL! (see my patch below).
>
> So simple. (I did think, at first, that deleting the '0' token was
> not a good idea - then I realized that it's more like skipping/ignoring
> the token than deleting it.)
Well ... I'm lazy, so ... and it gave me the garantee that it will
behave exactly like '{ }'.
> The patch below was (I think) my third attempt. If memory serves
> me, the first patch attempted to determine the '{0}' initializer
> from the 'struct expession *' passed to bad_null() alone. However,
> that did not allow me to distinguish '= { 0 }' from '= { 0, }',
> so I needed to backup from evaluation to the parse.
I think it's fine to allow the comma, I probably need to change
this is my version.
> Also, I didn't test the initialization of embedded struct/array fields
> (and what should happen anyway? should '{ 0 }' also work for initializing
> the sub-structure(s), or should it only work at the top-level?).
In fact, it works for literally anything: simple arrays, multi-dimensional
arrays (it must be because the braces doesn't need to match:
int a[2][2] = { 1, 2, 3, 4 };
is perfectly legal), structures with a scalar as first member, more complex
strutures, sub-structures, and more suprisingly even for simple types:
int a = { 0 };
_Bool b = { 0 };
double f = { 0 };
int *ptr = { 0 };
If it is fine for you, I'll reuse your testcases.
> Also, I have just noticed, it seems that I can't decide if it should
> be called 'zero aggregate initializer' or 'aggregate zero initializer'! :-P
I don't think it has a specfic name in the standard but has Danh said
in his reply, in some books, articles, GCC & clang patches it's
called "universal [zero] initializer".
Best regards,
-- Luc
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings
2020-05-20 20:40 ` Luc Van Oostenryck
@ 2020-05-20 22:03 ` Ramsay Jones
0 siblings, 0 replies; 6+ messages in thread
From: Ramsay Jones @ 2020-05-20 22:03 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: linux-sparse, Junio C Hamano, Đoàn Trần Công Danh
On 20/05/2020 21:40, Luc Van Oostenryck wrote:
> On Wed, May 20, 2020 at 01:22:22AM +0100, Ramsay Jones wrote:
[snip]
>>
>> I remember the discussion (on lkml and sparse ml) in which
>> there was general agreement that '{}' would be preferred
>> solution (if only it was standard C!). However, I thought
>> that (since some compilers don't support it e.g. msvc) the
>> next best solution would be for sparse to suppress the
>> warning if given the '= { 0 }' token sequence. (ie. no mention
>> of it being conditional on a option).
>
> Yes, I kinda agree but concerning the kernel, my understanding is
> that the warning is desired (cfr. https://marc.info/?t=154704602900003 )
Oh, my lord, I had no recollection of that thread - and it was
only just over a year ago! ;-P
Hmm, yes it's a shame, but I guess the kernel usage takes precedence.
> For example, for cases like:
> int *array[16] = { 0 };
>
> So, I want to keep the current behavior as the default.
>
>>> @@ -2750,6 +2750,13 @@ static struct token *initializer_list(struct expression_list **list, struct toke
>>> {
>>> struct expression *expr;
>>>
>>> + // '{ 0 }' is equivalent to '{ }' unless wanting all possible
>>> + // warnings about using '0' to initialize a null-pointer.
>>> + if (!Wuniversal_initializer) {
>>> + if (match_token_zero(token) && match_op(token->next, '}'))
>>> + token = token->next;
>>> + }
>>> +
>>
>> Ha! This made me LOL! (see my patch below).
>>
>> So simple. (I did think, at first, that deleting the '0' token was
>> not a good idea - then I realized that it's more like skipping/ignoring
>> the token than deleting it.)
>
> Well ... I'm lazy, so ... and it gave me the garantee that it will
> behave exactly like '{ }'.
>
>> The patch below was (I think) my third attempt. If memory serves
>> me, the first patch attempted to determine the '{0}' initializer
>> from the 'struct expession *' passed to bad_null() alone. However,
>> that did not allow me to distinguish '= { 0 }' from '= { 0, }',
>> so I needed to backup from evaluation to the parse.
>
> I think it's fine to allow the comma, I probably need to change
> this is my version.
No, No, that would definitely be wrong. In fact, I would go further
and say _only_ '= { 0 } ;' should suppress the warning (yes I added
the semi-colon). (I did think that maybe other forms of 'integer
constant with value zero' could be added; e.g. 0x0, but I am not
sure even that is useful).
['designated initializers' would also not work to suppress the
warnings, of course!]
BTW, I was not entirely convinced by the git-list discussion which
lead to this patch. However, limiting the suppression of the warning
to _just_ '= { 0 } ;' would leave the majority of use-cases issuing
the warning anyway. The main benefit would be, as argued by others,
that when you switch the order/type of fields in a struct (say) that
you would not have to change the initializer from/to {0}/{NULL}.
(Again, I don't see that as a huge advantage ...)
>> Also, I didn't test the initialization of embedded struct/array fields
>> (and what should happen anyway? should '{ 0 }' also work for initializing
>> the sub-structure(s), or should it only work at the top-level?).
And so, given the above, I don't think the warnings should be suppressed
on sub-structures.
> In fact, it works for literally anything: simple arrays, multi-dimensional
> arrays (it must be because the braces doesn't need to match:
> int a[2][2] = { 1, 2, 3, 4 };
Heh, yes indeed.
> is perfectly legal), structures with a scalar as first member, more complex
> strutures, sub-structures, and more suprisingly even for simple types:
> int a = { 0 };
> _Bool b = { 0 };
> double f = { 0 };
> int *ptr = { 0 };
Ah, yes, I wonder if that would be a problem. ;-)
My initial reaction would be that non-aggregate types would still
issue warnings (even with ={0};), but that starts getting harder
to do ... :(
I don't have any simple answers.
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings
2020-05-20 0:22 ` Ramsay Jones
2020-05-20 0:41 ` Đoàn Trần Công Danh
2020-05-20 20:40 ` Luc Van Oostenryck
@ 2020-06-02 16:41 ` Luc Van Oostenryck
2 siblings, 0 replies; 6+ messages in thread
From: Luc Van Oostenryck @ 2020-06-02 16:41 UTC (permalink / raw)
To: Ramsay Jones; +Cc: linux-sparse
On Wed, May 20, 2020 at 01:22:22AM +0100, Ramsay Jones wrote:
> >
> > diff --git a/parse.c b/parse.c
> > index a29c67c8cf41..48494afc6f2c 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -2750,6 +2750,13 @@ static struct token *initializer_list(struct expression_list **list, struct toke
> > {
> > struct expression *expr;
> >
> > + // '{ 0 }' is equivalent to '{ }' unless wanting all possible
> > + // warnings about using '0' to initialize a null-pointer.
> > + if (!Wuniversal_initializer) {
> > + if (match_token_zero(token) && match_op(token->next, '}'))
> > + token = token->next;
> > + }
> > +
>
> Ha! This made me LOL! (see my patch below).
>
> So simple. (I did think, at first, that deleting the '0' token was
> not a good idea - then I realized that it's more like skipping/ignoring
> the token than deleting it.)
>
> I wish I had thought of it.
Well, it ended that it wasn't that smart after all because it
caused several regressions when used with scalars.
So, I finally had to do a sort of hybrid between your version
(for the parsing) and mine (dropping the '0' element from the list,
but now, later, at evaluation time).
-- Luc
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-02 16:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18 23:54 [SPARSE PATCH] univ-init: conditionally accept { 0 } without warnings Luc Van Oostenryck
2020-05-20 0:22 ` Ramsay Jones
2020-05-20 0:41 ` Đoàn Trần Công Danh
2020-05-20 20:40 ` Luc Van Oostenryck
2020-05-20 22:03 ` Ramsay Jones
2020-06-02 16:41 ` Luc Van Oostenryck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.