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

[Alexander Kornienko]

Hi Chandler,

Here's the new patch. Changes in short: clang:: prefix for the attribute,
avoid fix-it hint when some fall-through paths are already annotated,
std::map -> llvm::SmallPtrSet, fixed indentation issues and minor wording
changes in comments.

On Thu, Apr 26, 2012 at 1:09 AM, Chandler Carruth <chandlerc at google.com>wrote:
>
> I think we should leave C++98 support for some later contributor to add if
> they are motivated to get this feature.
>
Yep, that's the way to go.

>     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.
>
Currently I have no idea how to implement this specific restriction. I
would suggest to leave it for one of the next iterations.


> 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.
>
Done. It turned out though, that namespaces for attributes were not used
until now, so I implemented my own vision: I just add namespace  as a
prefix to spelling with triple underscore delimiter. Better ideas?

 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.
>
Done.


>  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.
>
You're right. Now there's no fix-it hint when some fall-through paths to
the same case label are already annotated.

-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120426/a8f1fe0b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch-implicit-fallthrough4.diff
Type: application/octet-stream
Size: 21650 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120426/a8f1fe0b/attachment.obj>