[cfe-commits] [PATCH] Implicit fall-through between switch labels

[Matthieu Monrocq]

Le 23 avril 2012 15:05, Alexander Kornienko <alexfh at google.com> a écrit :

> Hello,
>
> Thanks everyone for comments!
>
> I'm sorry for being out of this thread for a while, but during this time
> we discussed this once again on IRC and came to an idea to change the
> design of this feature to be more clear and allow more fine-grained
> annotation of execution paths which lead to fall-through to the next switch
> labels.
>
> In current implementation the [[fallthrough]] attribute should be applied
> to a null statement placed in a point of execution where fall-through to
> the next switch label occurs, e.g.:
>
> switch (n) {
>   case 1:
>     if (x)
>       break;
>     else if (y) {
>       ...
>       [[fallthrough]];  // annotated fall-through to case 2
>     }
>     else
>       return 13;
>   case 2:               // no warning here
>     ...
>     [[fallthrough]];    // annotated fall-through to case 3
>   case 3:
>     ...
> }
>
> So, in the new design [[fallthrough]]; annotation can be used in mostly
> in the same way as break; or return; statements (but it doesn't change
> control-flow, it just annotates a fall-through). The following rules are
> checked for this annotation:
>  * fallthrough attribute can only be attached to a null-statement;
>  * [[fallthrough]]; annotation should be placed inside switch body;
>  * it should be placed on an execution path between any statement inside
> switch body and case/default label (this means there's fall-through to
> the corresponding case/default label);
>  * no statements should exist on an execution path between
> [[fallthrough]]; annotation and the next case/default label.
>
> The diagnostic wording was changed to be more common:
> ...
> switch (n) {
>   case 4:
>     ;
>   case 5:
> }
> ...
>
> test.cpp:19:5: warning: unannotated fall-through between switch labels;
> use [[fallthrough]]; attribute to silence this warning
> [-Wimplicit-fallthrough]
>     case 5:
>     ^
> test.cpp:18:9: note: fall-through from here; use [[fallthrough]];
> attribute to silence this warning [-Wimplicit-fallthrough]
>         ;
>         ^
>         [[fallthrough]];
>
>
Hello,

Just a tiny remark on the wording of the diagnostics: I would avoid using a
`;` after `[[fallthrough]]`. The attribute is `fallthrough`, the syntax for
attributes leads to `[[fallthrough]]`, but there is no `;` in general. And
similarly for the fix-it hint on the note when the `;` is already present.

This is looking really great, it'll be nice to be able to document
intentional fallthroughs :)

-- Matthieu


> Known issues:
>  * fix-it hint location has to be adjusted to the end of the statement
> using lexer.
>
> Please, review this patch.
>
> On Sun, Apr 22, 2012 at 4:48 AM, Jordy Rose <jediknil at belkadan.com> wrote:
>
>>
>> On Apr 21, 2012, at 20:40, Chandler Carruth wrote:
>>
>> > On Sat, Apr 21, 2012 at 5:10 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> >> On Sat, Apr 21, 2012 at 4:54 PM, Joe Groff <arcata at gmail.com> wrote:
>> >>> For future-proofing's sake, does the standard provide any guidance for
>> >>> naming nonstandardized attributes? Should the attribute be named
>> >>> something like 'clang::fallthrough' instead of just 'fallthrough', in
>> >>> case a future standard or other implementations provide for a similar
>> >>> attribute with different behavior?
>> >>>
>> >> Yes, I think so. The attribute namespace mechanism was designed to
>> allow such vendor extensions without creating problems for future
>> standardization.
>> >>
>> > I would find this extremely unfortunate. The fallthrough check doesn't
>> seem likely at all to be a vendor-specific extension. I can't imagine any
>> possible standardized meaning for the attribute other than the use being
>> proposed here. Forcing users to type 'clang::' in each place seems to
>> reduce the clarity and readability of the construct with no real benefit.
>> Is this really necessary? Can we not add a top-level attribute when it
>> makes sense?
>>
>> I'm with Chandler. The namespacing syntax would be useful for something
>> like analyzer_noreturn, but this is something other compiler vendors could
>> implement as well. I know we're supposed to be careful about adding
>> language extensions (should this discussion be on cfe-dev?) but this is
>> hardly a vendor-specific warning.
>>
>> Jordy
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
>
> --
> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221
> 77 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>
> AG Hamburg, HRB 86891 | Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Katherine Stephens
>
> Tax ID:- 48/725/00206
> VAT ID:- DE813741370
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120423/1f482eaa/attachment.html>