On Mon, 2016-08-29 at 10:04 -0400, Eric Gallager wrote:
> On 8/29/16, David Malcolm <dmalc...@redhat.com> wrote:
> > On Mon, 2016-08-29 at 09:32 -0400, Eric Gallager wrote:
> > > On 8/29/16, Marek Polacek <pola...@redhat.com> wrote:
> > > > Tobias tried my latest version and reported some ICEs. They
> > > > should
> > > > all be
> > > > fixed in this version (the only change since version 6 is the
> > > > cp/pt.c
> > > > hunk).
> > > >
> > > > At this point I'd like to ask Jason and Joseph to review the
> > > > C/C++
> > > > parts
> > > > and someone to review the ME parts so that I can finally wrap
> > > > this
> > > > thing
> > > > up. This warning found a couple of bugs in our codebase and I
> > > > suspect it
> > > > will find some in other codebases, too.
> > > >
> > > > Does anyone have any concerns that I haven't addressed yet?
> > > >
> > > > Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for
> > > > trunk?
> > > >
> > >
> > >
> > > I tried v6 on my binutils-gdb fork, and it printed A LOT of
> > > warnings... After this patch goes in, the main question I'd have
> > > would
> > > be about the carets: it seems like it would make more sense for
> > > me
> > > for
> > > the location of the warning to be swapped with that of the fixit
> > > hint(s)? With the fixit pointing to the case label before the
> > > statement that's warned about, it makes it look like it's
> > > suggesting
> > > to put the fallthrough attribute or the break before the rest of
> > > the
> > > content of the case, which, with the break, could lead to dead
> > > code.
> > > I'd think it'd make more sense to point to after the body of the
> > > case
> > > statement instead...
> >
> > Interesting. Please can you post an example of the output that
> > you're
> > referring to?
> >
> > I'm working on improvements to how we print fix-its, so I'm
> > wondering
> > if this is an issue with the fix-it data, or with the presentation
> > of
> > it.
> >
> > Thanks
> > Dave
> >
>
> Sure, an example of the warning looks like this for me:
>
> ./cli/cli-setshow.c: In function ‘do_setshow_command’:
> ./cli/cli-setshow.c:353:7: warning: this statement may fall through
> [-Wimplicit-fallthrough]
> if (*(unsigned int *)c->var == UINT_MAX)
> ^
> ./cli/cli-setshow.c:359:2: note: insert ‘__attribute__
> ((fallthrough));’ to silence this warning
> case var_zinteger:
> ^~~~
> __attribute__ ((fallthrough));
> ./cli/cli-setshow.c:359:2: note: insert ‘break;’ to avoid fall
> -through
> case var_zinteger:
> ^~~~
> break;
Thanks. So presumably you have something like:
... case something:
353 if (*(unsigned int *)c->var == UINT_MAX)
354 {
355
356 /* various code here */
357 }
358
359 case var_zinteger:
360 /* etc */
and hence the new warning is complaining about the fallthrough to line
359.
If so, I agree that the fix-it as it's currently printed is misleading.
Insertion fix-its appear immediately before the text at the caret, so
the result of applying the fix-its (using my -fdiagnostics-generate
-patch patchkit) would be:
359 __attribute__ ((fallthrough));case var_zinteger:
and:
359 break;case var_zinteger:
It looks like the fix-it data is technically correct, but the end-result
doesn't resemble normal formatting.
In fact, -fdiagnostics-generate-patch doesn't know about "apply this OR apply
that", so the generated patch would probably look like this:
@@ -354,5 +354,5
}
- case var_zinteger:
+ __attribute__ ((fallthrough));break;case var_zinteger:
/* etc */
The text inserted by the fixits probably ought to be on lines to themselves,
with appropriate indentation, so that the generated patch would then look
something like this:
@@ -354,5 +354,6
}
+ __attribute__ ((fallthrough));
case var_zinteger:
/* etc */
and:
@@ -354,5 +354,6
}
+ break;
case var_zinteger:
/* etc */
This is non-trivial for two reasons:
(a) there are various places in the fixit handling-code that aren't yet
expecting newlines in fixit text; in particular, printing the fix-its.
Also, diagnostics_show_locus also will currently only print fixits on lines
that are touched by a range within the diagnostic.
(b) figuring out the appropriate indentation may be hard
These issues are fixable, but seem something of a digression. I think the best
course of action here is for Marek to remove the fix-it hints for now, so it
doesn't block getting the warning into trunk, and for us to work on adding the
fixits as follow-up work once the less ambitious version of the patch is
approved. I believe that the "note" diagnostics make sense to print even
without the fixit hints, so that those could be kept for now.
Marek, others: does this sound like a good course of action?
Dave
