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

[Chandler Carruth]

On Wed, Apr 25, 2012 at 3:05 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Or maybe someone really interested in supporting this feature in C++ 98
> mode could offer any better idea?
>

I think we should leave C++98 support for some later contributor to add if
they are motivated to get this feature.


>
> >     case 223:          // expected-warning{{unannotated fall-through
>> between switch labels}}
>> >       [[fallthrough]]; // expected-warning{{fallthrough annotation does
>> not directly precede switch label}}
>> >   }
>> >   return t;
>> > }
>>
>> I'm not convinced the last case should warn. It's pretty common to put a
>> 'break;' in the last case statement (especially a non-default one) for
>> symmetry, and to defend against further changes. While it's a little less
>> conceivable that you would want to imply [[fallthrough]]; for future
>> changes, it still seems like a valid case for symmetry.
>>
> I thought of fall-through annotation as a last resort for situations when
> having fall-through is critical for performance or other (not really
> frequent) serious reasons. For me it's a goto-style thing, which should
> be avoided in most cases.
>

My problem with this is that the fallthrough actually *looks* less
goto-style. I would go further, I think we should treat the last case as-if
it had a case following it.

Consider that a common use case is making huge switches with many case
labels future proof to cases being moved or re-sorted. Without insisting on
either 'break;' or '[[fallthrough]];' here, the last case becomes specially
ignored by the warning. With a warning in the reverse, the last case grows
an inverted requirement from every other case.

> Even if we stick with allowing [[fallthrough]], should we /also/ allow
>> [[clang:fallthrough]]?
>>
> Not a bad idea, and it's quite popular in this discussion ;) I think I'll
> do this in the next iteration.
>

FWIW, Richard convinced me that this must be spelled 'clang::fallthrough'
for the immediate future. We should probably focus on that form.


> Rather than build a map of false/true pairs, then mark half of them true,
>> why not use an llvm::SmallPtrSet, building up the set in
>> VisitAttributedStmt and removing from it in MarkFallthroughValid?
>> Advantages: stack-based allocation if the number of fallthrough statements
>> is small, only one data structure, and no second traversal and no copying
>> when it comes time to warn about the invalid fallthroughs.
>>
> I'll look into this.
>

I think any of these data structure concerns should be addressed even in
the first iteration fwiw.

This is a deal-breaker for me; fixits should always create buildable code.
>> I'd be okay with this being fixed to add braces, or with the fallthrough
>> statement being pushed outside the if. It would also be nice to have a set
>> of fixit tests, to make sure we're not just warning but actually doing the
>> right thing.
>>
> I currently removed all artificial intelligence from this code, so it just
> suggests to insert a fall-through annotation just before a case label. This
> is more consistent (but it could also add warnings in a case like this:
>
> case 1:
>   if (x) {
>     ...;
>     [[fallthrough]];
>   } else {
>     ...;
>   }
> case 2:
>
> because adding an annotation before case 2 would render a previous
> annotation inside if(x) {..} to be redundant. But this is not a broken
> build, it's just a warning which doesn't affect semantics.
>

I don't understand why we can't simply suppress the fixit note when we have
a case like this one that it might be wrong for. Saying it's "just a
warning" doesn't help much if you use -Werror. My suggestion is to not
suggest a fixit hint if there are already [[fallthrough]] annotations in
the case somewhere, we're almost certain to get it wrong.


Thanks again, this is an exciting extension for Clang!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120425/42d43287/attachment.html>