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

Thu May 3 05:02:33 PDT 2012

Hi Richard,

Following your privately posted comments, I've made some changes in
punctuation for diagnostics, added documentation to
docs/LanguageExtensions.html and rebuilt patch against current HEAD. Here
it is.
I've also attached a small patch which removes unnecessary fall-throughs in
3 widely-used header files. Though it is a separate patch, it's definitely
related to the main one.
Thanks for your review!

On Wed, May 2, 2012 at 12:42 AM, Alexander Kornienko <alexfh at google.com>wrote:

> Hi Richard,
>
> Here's an updated patch.
>
> On Tue, May 1, 2012 at 5:28 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> Hi Alex,
>>
>> The 'unreachable block' test isn't quite right; a block can have
>> (unreachable) predecessors and still be unreachable:
>>
>> switch (x) {
>> // oops, deleted "case 0:"
>>   if (a)
>>     f();
>>   [[clang::fallthrough]]; // no warning here!
>> case 1:
>>   break;
>> }
>>
>>  That said, since -Wunreachable-code warns on (most) such cases, I don't
>> think that is necessarily a problem. It might be worth updating the comment
>> in the code to indicate that we only catch trivial cases, though.
>>
> I wrote in one of previous letters that this detects only trivially
> unreachable blocks. Now I added comment in the code. I think that this
> specific diagnostic can be helpful if -Wunreachable-code isn't in use.
>
>
>> Style nits: Please update the capitalization of FallthroughMapper's
>> members and local variables to match the coding standard. Functions should
>> start with a lowercase letter, and variables and data members should start
>> with a capital letter. Also, please start comments with a capital letter,
>> and end them with a full stop when they are full sentences.
>>
> Done. The only question is: is naming style enforced somehow?
> RecursiveASTVisitor.h, for example, uses different conventions.
>
>
>> On Mon, Apr 30, 2012 at 5:40 AM, Alexander Kornienko <alexfh at google.com>wrote:
>>
>>> On Fri, Apr 27, 2012 at 3:47 AM, Jordy Rose <jediknil at belkadan.com>wrote:
>>>
>>>> >> 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?
>>>>
>>>> The right thing might be to implement real namespacing now, but why not
>>>> just use ::, as in the written form? The parser only expects attributes to
>>>> be made of identifiers, but our table can use any characters we want.
>>>>
>>>
>>> These strings are used as parts of C++ identifiers in clang code, so we
>>> can't use arbitrary characters inside. I found underscores to be the most
>>> appropriate option here. Regarding correct implementation of namespacing in
>>> attributes, I strongly believe it's an independent piece of functionality,
>>> which requires a separate design discussion. So I would leave my temporary
>>> implementation (which is quite flexible to be used at the moment).
>>>
>>
>> Since you're planning on working on fixing this next, I think the
>> temporary implementation is OK. Please add a FIXME next to it, though.
>>
> Added.
>
>
>> > +    typedef std::vector<const Stmt*> StmtVec;
>>>
>>> > +    bool GetFallThroughSourceStmts(const CFGBlock &B, StmtVec
>>>> &Unannotated,
>>>> > +                                   StmtVec &Annotated) {
>>>>
>>>> Should this be a SmallVector? The argument would then be a
>>>> SmallVectorImpl<const Stmt*>, and only the actual stack-allocated vector
>>>> needs to have a size.
>>>>
>>>> Also, do we need the annotated set? If not, we could leave it out for
>>>> now, and only put it back in if we want to try for some intelligence in the
>>>> suggestions.
>>>>
>>> I changed this to return just counts of annotated and unannotated
>>> fall-through source locations
>>>
>>
>> I notice you don't use UnannotatedCnt outside GetFallThroughSourceCount.
>> I suggest you drop that argument, and replace UnannotatedCnt with a bool
>> inside the function. AnnotatedCnt could likewise be replaced by a bool
>> FoundAnyFallthroughAttributes or similar.
>>
> Removed UnannotatedCnt from parameters, but left AnnotatedCnt as int.
> Don't think this will hurt performance and code readability, but this code
> can be easier changed to provide more detailed diagnostics.
>
> I'd also suggest renaming that function, perhaps to
>> CheckFallThroughIntoBlock.
>>
> You mean checkFallThroughIntoBlock? ;) Done.
>
>
>>  > +      SourceLocation L = Label->getLocStart();
>>>> > +      S.Diag(L, diag::note_insert_fallthrough_fixit) <<
>>>> > +        FixItHint::CreateInsertion(L, "[[clang::fallthrough]];");
>>>>
>>>> This will give a proper diagnostic, but not a very nice placement of
>>>> the attribute. Clients like Xcode actually apply fixit hints to the user's
>>>> code, and really we want this on a previous line. Can we figure out how to
>>>> insert this in the right place, rather than suggest something the user will
>>>> have to manually edit afterwards?
>>>
>>> Proper formatting should be a function of an IDE, not a compiler. If
>>> Xcode uses clang's fix-it hints for changing code, it could also deal with
>>> white-space handling.
>>> I've inserted a line-break at the end of fix-it hint, but I'm not really
>>> sure if even this is a duty of the compiler. And AFAIK there are no other
>>> examples of fix-it hints, which insert new statements or deal with code
>>> formatting in any other way.
>>>
>>
>> We do have existing fixits which insert spaces and newlines to keep the
>> code tidy, in cases where "tidy" is obvious, but I agree that it can't be
>> the responsibility of the fixit to reformat the code according to whatever
>> coding style is relevant. This fixit seems like a bit of a grey area, since
>> "keep the case label at the same indentation, and put the attribute at the
>> same indentation as the previous line" is likely to cover almost all cases
>> perfectly, but I think what you've got now is a reasonable tradeoff. And I
>> don't want for us to need to worry about what happens if 'case' or
>> 'default' isn't the first token on the line :-)
>>
> I even had to remove '\n' from my fix-it hints, 'cause \n's caused some
> assertion fails inside diagnostic rendering. I suggest start using this
> diagnostic and then figure out what fix-it spelling is the best here.
>
>
>> Other than the superficial issues above, I think this is looking good.
>>
>> Thanks!
>> Richard
>>
>
-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120503/4a6dc0b6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch-implicit-fallthrough7.diff
Type: application/octet-stream
Size: 31057 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120503/4a6dc0b6/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch-implicit-fallthrough-fixes.diff
Type: application/octet-stream
Size: 1575 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120503/4a6dc0b6/attachment-0001.obj>