On Mon, Aug 29, 2016 at 10:41:57AM -0400, David Malcolm wrote:
> 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.
>From the very beginning, my intent was to show *what* falls through *where*,
so I think the locations are precise as they are and I wouldn't want to
change them.
> 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?
Yeah, I appreciate this isn't easy. Given fix-it hints shouldn't be considered
a blocker for a patch like this, I can simply resort to what I had in the
early phase, that is say something like
q.c:7:8: warning: this statement may fall through [-Wimplicit-fallthrough]
b++;
~^~
q.c:8:5: note: here
case 1:
^~~~
and we can improve things incrementally.
Thanks,
Marek
