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

[Alexander Kornienko]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120430/785ead45/attachment.html>