Re: [PATCH] C: fixits for modernizing structure member designators
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
Re: [PATCH] C: fixits for modernizing structure member designators
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
Re: [PATCH] C: fixits for modernizing structure member designators
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...
Re: [PATCH] C: fixits for modernizing structure member designators
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
Re: [PATCH] C: fixits for modernizing structure member designators
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
Re: [PATCH] C: fixits for modernizing structure member designators
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
Re: [PATCH] C: fixits for modernizing structure member designators
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
