[PATCH] Treat { 0 } specially for structs with the designated_init attribute.
Martin Sebor
msebor@gmail.com
Mon Jun 8 17:27:56 GMT 2020
On 6/3/20 9:57 PM, Asher Gordon via Gcc-patches wrote:
> Hello,
>
> I accidentally wrote 'free(loc)' instead of 'free (loc)'. Please see the
> fixed patch attached below (contrib/check_GNU_style.sh says it's OK
> now):
>
>
> 0001-Treat-0-specially-for-structs-with-the-designated_in.patch
>
> From 0445fba96ee9030feb00ebec893f8dfed153b12d Mon Sep 17 00:00:00 2001
> From: Asher Gordon<AsDaGo@posteo.net>
> Date: Wed, 3 Jun 2020 17:20:08 -0400
> Subject: [PATCH] Treat { 0 } specially for structs with the designated_init
> attribute.
>
> Closes #95379.
I think the idea behind this change is fine. I just have a few
minor comments on the implementation. A C front end maintainer
will need to formally review the patch.
As an aside, the bug number should be referenced in the change
message so that when the patch is committed it's automatically
reflected in the bug entry. The contrib/mklog.py script should
do that automatically based on new test files added to verify
the bug fix/change, provided one such test is added and starts
with a PR c/95379 comment. Also mentioning it in the Subject
of the email is helpful.
>
> gcc/ChangeLog:
>
> * doc/extend.texi: Document { 0 } as a special case for the
> designated_init attribute.
>
> gcc/c/ChangeLog:
>
> * c-typeck.c (struct location_list): New type.
> (struct initializer_stack): Add positional_init_locs for
> -Wdesignated-init warnings.
> (start_init): Initialize initializer_stack->positional_init_locs
> to NULL.
> (finish_init): Free initializer_stack->positional_init_locs.
> (pop_init_level): Move -Wdesignated-init warning here from
> process_init_element so that we can treat { 0 } specially.
> (process_init_element): Instead of warning on -Wdesignated-init
> here, remember a list of locations where we should warn and do the
> actual warning in pop_init_level.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/Wdesignated-init.c: Add tests for ignoring { 0 }.
I don't think there's any "rule" against it but it's customary
to add test files for new features, rather than modify existing
ones (see also my comment about the post-commit hook above).
> ---
> gcc/ChangeLog | 5 +++
> gcc/c/ChangeLog | 14 +++++++
ChangeLog changes need not be included in a patch (and shouldn't
be). They are now automatically extracted from the commit message
and added to the ChangeLog files by a post-commit hook.
> gcc/c/c-typeck.c | 55 +++++++++++++++++++++++--
> gcc/doc/extend.texi | 4 ++
> gcc/testsuite/ChangeLog | 4 ++
> gcc/testsuite/gcc.dg/Wdesignated-init.c | 3 ++
> 6 files changed, 81 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index edbcaf2bc4d..fd60a248226 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -95,6 +95,11 @@
> (lto_tree_code_to_tag): Update.
> (lto_tag_to_tree_code): Update.
>
> +2020-06-03 Asher Gordon<AsDaGo@posteo.net>
> +
> + * doc/extend.texi: Document { 0 } as a special case for the
> + designated_init attribute.
> +
> 2020-06-02 Felix Yang<felix.yang@huawei.com>
>
> PR target/95459
> diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog
> index abf31e57688..e77f46930ef 100644
> --- a/gcc/c/ChangeLog
> +++ b/gcc/c/ChangeLog
> @@ -17,6 +17,20 @@
>
> * c-objc-common.h (LANG_HOOKS_OMP_PREDETERMINED_MAPPING): Redefine.
>
> +2020-06-03 Asher Gordon<AsDaGo@posteo.net>
> +
> + * c-typeck.c (struct location_list): New type.
> + (struct initializer_stack): Add positional_init_locs for
> + -Wdesignated-init warnings.
> + (start_init): Initialize initializer_stack->positional_init_locs
> + to NULL.
> + (finish_init): Free initializer_stack->positional_init_locs.
> + (pop_init_level): Move -Wdesignated-init warning here from
> + process_init_element so that we can treat { 0 } specially.
> + (process_init_element): Instead of warning on -Wdesignated-init
> + here, remember a list of locations where we should warn and do the
> + actual warning in pop_init_level.
> +
> 2020-05-28 Nicolas Bértolo<nicolasbertolo@gmail.com>
>
> * Make-lang.in: Remove extra slash.
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 385bf3a1c7b..2d04f70f0cf 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -8179,6 +8179,13 @@ struct constructor_range_stack
>
> static struct constructor_range_stack *constructor_range_stack;
>
> +/* A list of locations. */
> +
> +struct location_list {
> + struct location_list *next;
> + location_t loc;
> +};
Rather than creating own linked list I suggest to consider making
use of one of GCC's data structures.
> +
> /* This stack records separate initializers that are nested.
> Nested initializers can't happen in ANSI C, but GNU C allows them
> in cases like { ... (struct foo) { ... } ... }. */
> @@ -8197,6 +8204,7 @@ struct initializer_stack
> char require_constant_value;
> char require_constant_elements;
> rich_location *missing_brace_richloc;
> + struct location_list *positional_init_locs;
> };
>
> static struct initializer_stack *initializer_stack;
> @@ -8222,6 +8230,7 @@ start_init (tree decl, tree asmspec_tree ATTRIBUTE_UNUSED, int top_level,
> p->top_level = constructor_top_level;
> p->next = initializer_stack;
> p->missing_brace_richloc = richloc;
> + p->positional_init_locs = NULL;
> initializer_stack = p;
>
> constructor_decl = decl;
> @@ -8287,6 +8296,13 @@ finish_init (void)
> spelling_size = p->spelling_size;
> constructor_top_level = p->top_level;
> initializer_stack = p->next;
> +
> + while (p->positional_init_locs)
> + {
> + struct location_list *list = p->positional_init_locs;
> + p->positional_init_locs = list->next;
> + free (list);
If the argument to free was returned from XNEW it should probably
be released by XDELETE (even if the two do the same same thing).
But using an existing container class would obviate having to deal
with these details.
> + }
> free (p);
> }
>
> @@ -8744,6 +8760,22 @@ pop_init_level (location_t loc, int implicit,
> }
> }
>
> + /* Warn when positional initializers are used for a structure with
> + the designated_init attribute, but make an exception for { 0 }. */
> + while (initializer_stack->positional_init_locs)
> + {
> + struct location_list *loc = initializer_stack->positional_init_locs;
> +
> + if (!constructor_zeroinit)
> + warning_init (loc->loc,
> + OPT_Wdesignated_init,
> + "positional initialization of field in %<struct%> "
> + "declared with %<designated_init%> attribute");
I realize the patch doesn't change the text of the message but...
if the name of the field is known at this point, mentioning it in
the message would be a helpful touch. If it isn't, then "a field"
would be grammatically more correct. In addition, pointing to
the definition of the struct in a follow-on note would also be
helpful (and in line with other similar messages). These
improvements are unrelated to the change you're making so could
be made separately, but since you're already making changes here
it's an opportunity to make them now (otherwise they're unlikely
to happen).
Martin
> +
> + initializer_stack->positional_init_locs = loc->next;
> + free (loc);
> + }
> +
> /* Pad out the end of the structure. */
> if (p->replacement_value.value)
> /* If this closes a superfluous brace pair,
> @@ -9975,10 +10007,25 @@ process_init_element (location_t loc, struct c_expr value, bool implicit,
> && TREE_CODE (constructor_type) == RECORD_TYPE
> && lookup_attribute ("designated_init",
> TYPE_ATTRIBUTES (constructor_type)))
> - warning_init (loc,
> - OPT_Wdesignated_init,
> - "positional initialization of field "
> - "in %<struct%> declared with %<designated_init%> attribute");
> + {
> + struct location_list *new_loc;
> +
> + /* Remember where we got a positional initializer so we can warn
> + if it initializes a field of a structure with the
> + designated_init attribute. */
> + new_loc = XNEW (struct location_list);
> + new_loc->next = NULL;
> + new_loc->loc = loc;
> + if (initializer_stack->positional_init_locs)
> + {
> + struct location_list *last = initializer_stack->positional_init_locs;
> + while (last->next)
> + last++;
> + last->next = new_loc;
> + }
> + else
> + initializer_stack->positional_init_locs = new_loc;
> + }
>
> /* If we've exhausted any levels that didn't have braces,
> pop them now. */
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index cced19d2018..a3c5059a8b0 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -8093,6 +8093,10 @@ attribute is to allow the programmer to indicate that a structure's
> layout may change, and that therefore relying on positional
> initialization will result in future breakage.
>
> +Note that the universal zero initializer, @code{@{ 0 @}}, is considered
> +a special case and will not produce a warning when used to initialize a
> +structure with the @code{designated_init} attribute.
> +
> GCC emits warnings based on this attribute by default; use
> @option{-Wno-designated-init} to suppress them.
>
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 89a1ad55923..a155c3c5332 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -67,6 +67,10 @@
> PR middle-end/94874
> * c-c++-common/gomp/pr94874.c: New.
>
> +2020-06-03 Asher Gordon<AsDaGo@posteo.net>
> +
> + * gcc.dg/Wdesignated-init.c: Add tests for ignoring { 0 }.
> +
> 2020-06-02 David Malcolm<dmalcolm@redhat.com>
>
> PR jit/95426
> diff --git a/gcc/testsuite/gcc.dg/Wdesignated-init.c b/gcc/testsuite/gcc.dg/Wdesignated-init.c
> index b9ca572206c..adb7949df55 100644
> --- a/gcc/testsuite/gcc.dg/Wdesignated-init.c
> +++ b/gcc/testsuite/gcc.dg/Wdesignated-init.c
> @@ -24,6 +24,9 @@ struct Des {
> struct Des d1 = { 5, 5 }; /* { dg-warning "(positional|near initialization)" } */
> struct Des d2 = { .x = 5, .y = 5 };
> struct Des d3 = { .x = 5, 5 }; /* { dg-warning "(positional|near initialization)" } */
> +struct Des d4 = { 0 };
> +struct Des d5 = { 5 }; /* { dg-warning "(positional|near initialization)" } */
> +struct Des d6 = { 0, 0 }; /* { dg-warning "(positional|near initialization)" } */
>
> struct Des fd1 (void)
> {
> -- 2.26.2
>
>
> Thanks,
> Asher
>
> -- <cas> well there ya go. say something stupid in irc and have it
> immortalised forever in someone's .sig file -------- I prefer to send
> and receive mail encrypted. Please send me your public key, and if you
> do not have my public key, please let me know. Thanks. GPG fingerprint:
> 38F3 975C D173 4037 B397 8095 D4C9 C4FC 5460 8E68
>
More information about the Gcc-patches
mailing list