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

[Alexander Kornienko]

Oh, and the patch itself ;)

On Mon, Apr 30, 2012 at 2:40 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Hi,
>
> Here's the new version of my patch. Changes:
>  * added fix-it hint 'did you forget ; ?' for missing semicolon in after
> attribute: [[clang::fallthrough]] case X:;
>  * added fix-it hint 'insert break; to avoid fall-through' - as a second
> option, which changes semantic of code, but is the most probable missed
> thing (it can be applied almost always without introducing more warnings,
> and it is unambiguous, what you can't say about return and throw);
>  * offer [[clang::fallthrough]] only in C++11 mode;
>  * don't offer any fix-its when macros are involved - it's hard to get it
> right, let the developer think on his own. a couple of test-cases included
> to show not-so-obvious cases;
>  * return counts instead of vectors<Stmt*>
> from FallthroughMapper::GetFallThroughSourceCount, as statements are
> currently not used.
>
> I hope that latest changes solve all serious problems and this version is
> already quite solid. Please review it.
> Thanks.
>
> 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).
>
>
>> My other suggestion for cramming two values in one field is a null
>> character, but that might have weird interactions with some of our current
>> infrastructure.
>>
> See my previous comment.
>
>
>> > +  SmallVector<char, 20> Buf;
>> > +  if (ScopeName)
>> > +    AttrName = (ScopeName->getName() + "___" +
>> AttrName).toStringRef(Buf);
>> > +
>>
>> I had to figure out this is building a Twine. It's probably not a big
>> issue, but I would probably change this to use SmallString (basically a
>> nice specialization of SmallVector for char) and += all the pieces.
>>
> I really don't know what the difference in performance would be for these
> two options, but changed these lines to the variant you suggested. At least
> it shouldn't be slower ;).
>
>
>> > +    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
>
>
>> > +      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.
>
> --
> Best regards,
> Alexander Kornienko
>
>


-- 
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120430/606c2205/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: switch-implicit-fallthrough5.diff
Type: application/octet-stream
Size: 28346 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120430/606c2205/attachment.obj>