| Message ID | 1472567926-12024-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/30/2016 04:38 PM, David Malcolm wrote: > In conjunction with the not-yet-in-trunk -fdiagnostics-generate-patch, > this can generate patches like this: > > --- modernize-named-inits.c > +++ modernize-named-inits.c > @@ -16,7 +16,7 @@ > /* Old-style named initializers. */ > > struct foo old_style_f = { > - foo: 1, > - bar: 2, > + .foo= 1, > + .bar= 2, > }; What happens if there are newlines in between any of the tokens? Bernd
On Tue, Aug 30, 2016 at 04:40:57PM +0200, Bernd Schmidt wrote: > On 08/30/2016 04:38 PM, David Malcolm wrote: > > > In conjunction with the not-yet-in-trunk -fdiagnostics-generate-patch, > > this can generate patches like this: > > > > --- modernize-named-inits.c > > +++ modernize-named-inits.c > > @@ -16,7 +16,7 @@ > > /* Old-style named initializers. */ > > > > struct foo old_style_f = { > > - foo: 1, > > - bar: 2, > > + .foo= 1, > > + .bar= 2, > > }; > > What happens if there are newlines in between any of the tokens? It's easy to check for yourself: when the identifier and colon are not on the same line, we print something like w.c: In function ‘main’: w.c:7:1: warning: obsolete use of designated initializer with ‘:’ [-Wpedantic] : ^ = which I don't think is desirable -- giving up on the fix-it hint in such case could be appropriate. I admit I dislike the lack of a space before = in ".bar= 2", but using richloc.add_fixit_replace (colon_loc, " ="); wouldn't work for "foo : 1" I think. Marek
On Tue, 2016-08-30 at 16:50 +0200, Marek Polacek wrote: > On Tue, Aug 30, 2016 at 04:40:57PM +0200, Bernd Schmidt wrote: > > On 08/30/2016 04:38 PM, David Malcolm wrote: > > > > > In conjunction with the not-yet-in-trunk -fdiagnostics-generate > > > -patch, > > > this can generate patches like this: > > > > > > --- modernize-named-inits.c > > > +++ modernize-named-inits.c > > > @@ -16,7 +16,7 @@ > > > /* Old-style named initializers. */ > > > > > > struct foo old_style_f = { > > > - foo: 1, > > > - bar: 2, > > > + .foo= 1, > > > + .bar= 2, > > > }; > > > > What happens if there are newlines in between any of the tokens? > > It's easy to check for yourself: when the identifier and colon are > not > on the same line, we print something like > > w.c: In function ‘main’: > w.c:7:1: warning: obsolete use of designated initializer with ‘:’ [ > -Wpedantic] > : > ^ > = > > which I don't think is desirable -- giving up on the fix-it hint in > such case > could be appropriate. I think that's a bug in diagnostic-show-locus.c; currently it only prints lines and fixits for the lines references in the ranges within the rich_location. I'll try to fix that. > I admit I dislike the lack of a space before = in ".bar= 2", but > using > > richloc.add_fixit_replace (colon_loc, " ="); > > wouldn't work for "foo : 1" I think. I actually tried that first, but I didn't like the way it was printed e.g.: w.c: In function ‘main’:> w.c:7:1: warning: obsolete use of designated initializer with ‘:’ [-Wpedantic]> foo: 1, ^ . = I'm looking at rewriting how fixits get printed, to print the affected range of the affected lines (using the edit-context.c work posted last week), so this would appear as: foo: 1, ^ .foo = Also, there may be a case for adding some smarts to gcc_rich_location for adding fixits in a formatting-aware way, by looking at the neighboring whitespace (this might help for the issue with adding "break;" etc in the fallthru patch kit). Dave
On Tue, Aug 30, 2016 at 11:22:21AM -0400, David Malcolm wrote: > On Tue, 2016-08-30 at 16:50 +0200, Marek Polacek wrote: > > On Tue, Aug 30, 2016 at 04:40:57PM +0200, Bernd Schmidt wrote: > > > On 08/30/2016 04:38 PM, David Malcolm wrote: > > > > > > > In conjunction with the not-yet-in-trunk -fdiagnostics-generate > > > > -patch, > > > > this can generate patches like this: > > > > > > > > --- modernize-named-inits.c > > > > +++ modernize-named-inits.c > > > > @@ -16,7 +16,7 @@ > > > > /* Old-style named initializers. */ > > > > > > > > struct foo old_style_f = { > > > > - foo: 1, > > > > - bar: 2, > > > > + .foo= 1, > > > > + .bar= 2, > > > > }; > > > > > > What happens if there are newlines in between any of the tokens? > > > > It's easy to check for yourself: when the identifier and colon are > > not > > on the same line, we print something like > > > > w.c: In function ‘main’: > > w.c:7:1: warning: obsolete use of designated initializer with ‘:’ [ > > -Wpedantic] > > : > > ^ > > = > > > > which I don't think is desirable -- giving up on the fix-it hint in > > such case > > could be appropriate. > > I think that's a bug in diagnostic-show-locus.c; currently it only > prints lines and fixits for the lines references in the ranges within > the rich_location. I'll try to fix that. > > > I admit I dislike the lack of a space before = in ".bar= 2", but > > using > > > > richloc.add_fixit_replace (colon_loc, " ="); > > > > wouldn't work for "foo : 1" I think. > > I actually tried that first, but I didn't like the way it was printed > e.g.: > > w.c: In function ‘main’:> w.c:7:1: warning: obsolete use of designated > initializer with ‘:’ [-Wpedantic]> > foo: 1, > ^ > . = > > I'm looking at rewriting how fixits get printed, to print the affected > range of the affected lines (using the edit-context.c work posted last > week), so this would appear as: > > foo: 1, > ^ > .foo = This would be perfect. > Also, there may be a case for adding some smarts to gcc_rich_location for adding fixits in a formatting-aware way, by looking at the neighboring whitespace (this might help for the issue with adding "break;" etc in the fallthru patch kit). Thanks. I hope it won't be too hard to implement :/ Marek
On Wed, 2016-08-31 at 11:09 +0200, Marek Polacek wrote: > On Tue, Aug 30, 2016 at 11:22:21AM -0400, David Malcolm wrote: > > On Tue, 2016-08-30 at 16:50 +0200, Marek Polacek wrote: > > > On Tue, Aug 30, 2016 at 04:40:57PM +0200, Bernd Schmidt wrote: > > > > On 08/30/2016 04:38 PM, David Malcolm wrote: > > > > > > > > > In conjunction with the not-yet-in-trunk -fdiagnostics > > > > > -generate > > > > > -patch, > > > > > this can generate patches like this: > > > > > > > > > > --- modernize-named-inits.c > > > > > +++ modernize-named-inits.c > > > > > @@ -16,7 +16,7 @@ > > > > > /* Old-style named initializers. */ > > > > > > > > > > struct foo old_style_f = { > > > > > - foo: 1, > > > > > - bar: 2, > > > > > + .foo= 1, > > > > > + .bar= 2, > > > > > }; > > > > > > > > What happens if there are newlines in between any of the > > > > tokens? > > > > > > It's easy to check for yourself: when the identifier and colon > > > are > > > not > > > on the same line, we print something like > > > > > > w.c: In function ‘main’: > > > w.c:7:1: warning: obsolete use of designated initializer with ‘:’ > > > [ > > > -Wpedantic] > > > : > > > ^ > > > = > > > > > > which I don't think is desirable -- giving up on the fix-it hint > > > in > > > such case > > > could be appropriate. > > > > I think that's a bug in diagnostic-show-locus.c; currently it only > > prints lines and fixits for the lines references in the ranges > > within > > the rich_location. I'll try to fix that. > > > > > I admit I dislike the lack of a space before = in ".bar= 2", but > > > using > > > > > > richloc.add_fixit_replace (colon_loc, " ="); > > > > > > wouldn't work for "foo : 1" I think. > > > > I actually tried that first, but I didn't like the way it was > > printed > > e.g.: > > > > w.c: In function ‘main’:> w.c:7:1: warning: obsolete use of > > designated > > initializer with ‘:’ [-Wpedantic]> > > foo: 1, > > ^ > > . = > > > > I'm looking at rewriting how fixits get printed, to print the > > affected > > range of the affected lines (using the edit-context.c work posted > > last > > week), so this would appear as: > > > > foo: 1, > > ^ > > .foo = > > This would be perfect. BTW, this is currently blocked, I need review of this patch: "[PATCH 2/4] Improvements to typed_splay_tree" * https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01777.html (I've rewritten edit_context to work on a per-line basis, so it should now be efficient enough for this use-case) > > Also, there may be a case for adding some smarts to > > gcc_rich_location for adding fixits in a formatting-aware way, by > > looking at the neighboring whitespace (this might help for the > > issue with adding "break;" etc in the fallthru patch kit). > > Thanks. I hope it won't be too hard to implement :/ I'll try...
On 08/30/2016 08:38 AM, David Malcolm wrote: > This patch adds fix-it hints to our warning for old-style structure > member designators, showing how to modernize them to C99 form. > > For example: > > modernize-named-inits.c:19:5: warning: obsolete use of designated initializer with ‘:’ [-Wpedantic] > foo: 1, > ^ > . = > > In conjunction with the not-yet-in-trunk -fdiagnostics-generate-patch, > this can generate patches like this: > > --- modernize-named-inits.c > +++ modernize-named-inits.c > @@ -16,7 +16,7 @@ > /* Old-style named initializers. */ > > struct foo old_style_f = { > - foo: 1, > - bar: 2, > + .foo= 1, > + .bar= 2, > }; > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > OK for trunk? > > gcc/c/ChangeLog: > * c-parser.c (c_parser_initelt): Provide fix-it hints for > modernizing old-style structure member designator to C99 style. > > gcc/testsuite/ChangeLog: > * gcc.dg/modernize-named-inits.c: New test case. OK. jeff
On Thu, 2016-09-15 at 11:27 -0600, Jeff Law wrote: > On 08/30/2016 08:38 AM, David Malcolm wrote: > > This patch adds fix-it hints to our warning for old-style structure > > member designators, showing how to modernize them to C99 form. > > > > For example: > > > > modernize-named-inits.c:19:5: warning: obsolete use of designated > > initializer with ‘:’ [-Wpedantic] > > foo: 1, > > ^ > > . = > > > > In conjunction with the not-yet-in-trunk -fdiagnostics-generate > > -patch, > > this can generate patches like this: > > > > --- modernize-named-inits.c > > +++ modernize-named-inits.c > > @@ -16,7 +16,7 @@ > > /* Old-style named initializers. */ > > > > struct foo old_style_f = { > > - foo: 1, > > - bar: 2, > > + .foo= 1, > > + .bar= 2, > > }; > > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > > > OK for trunk? > > > > gcc/c/ChangeLog: > > * c-parser.c (c_parser_initelt): Provide fix-it hints for > > modernizing old-style structure member designator to C99 style. > > > > gcc/testsuite/ChangeLog: > > * gcc.dg/modernize-named-inits.c: New test case. > OK. > jeff Thanks. Marek expressed some concern about how the fix-it hint is printed: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg02112.html I've been looking at rewriting how fixits get printed, to print the affected range of the affected lines, which would address his concern, but this work isn't finished yet. Should that work be a blocker for committing this modernize-named -inits.c fix-it patch, or is the patch good enough as-is? (for example, -fdiagnostics-generate-patch does a good job on it as -is). Dave
--- modernize-named-inits.c +++ modernize-named-inits.c @@ -16,7 +16,7 @@ /* Old-style named initializers. */ struct foo old_style_f = { - foo: 1, - bar: 2, + .foo= 1, + .bar= 2, }; Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. OK for trunk? gcc/c/ChangeLog: * c-parser.c (c_parser_initelt): Provide fix-it hints for modernizing old-style structure member designator to C99 style. gcc/testsuite/ChangeLog: * gcc.dg/modernize-named-inits.c: New test case. --- gcc/c/c-parser.c | 17 +++++++----- gcc/testsuite/gcc.dg/modernize-named-inits.c | 39 ++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/modernize-named-inits.c diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index c245e70..a6281fc 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -4459,13 +4459,18 @@ c_parser_initelt (c_parser *parser, struct obstack * braced_init_obstack) && c_parser_peek_2nd_token (parser)->type == CPP_COLON) { /* Old-style structure member designator. */ - set_init_label (c_parser_peek_token (parser)->location, - c_parser_peek_token (parser)->value, - c_parser_peek_token (parser)->location, - braced_init_obstack); + location_t name_loc = c_parser_peek_token (parser)->location; + tree fieldname = c_parser_peek_token (parser)->value; + location_t colon_loc = c_parser_peek_2nd_token (parser)->location; + set_init_label (name_loc, fieldname, name_loc, braced_init_obstack); /* Use the colon as the error location. */ - pedwarn (c_parser_peek_2nd_token (parser)->location, OPT_Wpedantic, - "obsolete use of designated initializer with %<:%>"); + rich_location richloc (line_table, colon_loc); + /* Provide fix-it hints for converting to C99 style i.e. + from "NAME:" to ".NAME =". */ + richloc.add_fixit_insert (name_loc, "."); + richloc.add_fixit_replace (colon_loc, "="); + pedwarn_at_rich_loc (&richloc, OPT_Wpedantic, + "obsolete use of designated initializer with %<:%>"); c_parser_consume_token (parser); c_parser_consume_token (parser); } diff --git a/gcc/testsuite/gcc.dg/modernize-named-inits.c b/gcc/testsuite/gcc.dg/modernize-named-inits.c new file mode 100644 index 0000000..4e16494 --- /dev/null +++ b/gcc/testsuite/gcc.dg/modernize-named-inits.c @@ -0,0 +1,39 @@ +/* { dg-do compile } */ +/* { dg-options "-fdiagnostics-show-caret -std=c99 -pedantic" } */ + +struct foo +{ + int foo; + int bar; +}; + +union u +{ + int color; + int shape; +}; + +/* Old-style named initializers. */ + +struct foo old_style_f = { + foo: 1, /* { dg-warning "5: obsolete use of designated initializer with ':'" } */ +/* { dg-begin-multiline-output "" } + foo: 1, + ^ + . = + { dg-end-multiline-output "" } */ + + bar : 1, /* { dg-warning "9: obsolete use of designated initializer with ':'" } */ + /* { dg-begin-multiline-output "" } + bar : 1, + ^ + . = + { dg-end-multiline-output "" } */ +}; + +union u old_style_u = { color: 3 }; /* { dg-warning "30: obsolete use of designated initializer with ':'" } */ +/* { dg-begin-multiline-output "" } + union u old_style_u = { color: 3 }; + ^ + . = + { dg-end-multiline-output "" } */
This patch adds fix-it hints to our warning for old-style structure member designators, showing how to modernize them to C99 form. For example: modernize-named-inits.c:19:5: warning: obsolete use of designated initializer with ‘:’ [-Wpedantic] foo: 1, ^ . = In conjunction with the not-yet-in-trunk -fdiagnostics-generate-patch, this can generate patches like this: