Oh, and the patch itself ;)<br><br><div class="gmail_quote">On Mon, Apr 30, 2012 at 2:40 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<div><br></div><div>Here's the new version of my patch. Changes:</div><div> * added fix-it hint 'did you forget ; ?' for missing semicolon in after attribute: <font face="courier new, monospace">[[clang::fallthrough]] case X:</font><font face="arial, helvetica, sans-serif">;</font></div>
<div> * 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);</div>
<div> * offer [[clang::fallthrough]] only in C++11 mode;</div><div> * 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;</div>
<div> * return counts instead of vectors<Stmt*> from FallthroughMapper::GetFallThroughSourceCount, as statements are currently not used.</div><div><br></div><div>I hope that latest changes solve all serious problems and this version is already quite solid. Please review it.</div>
<div>Thanks.</div><div><br></div><div><div class="gmail_quote"><div class="im">On Fri, Apr 27, 2012 at 3:47 AM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>>> FWIW, Richard convinced me that this must be spelled 'clang::fallthrough' for the immediate future. We should probably focus on that form.<br>
><br>
> 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?<br>
<br>
</div>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.<br>
</blockquote><div><br></div></div><div>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).</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br></blockquote></div><div>See my previous comment.</div><div class="im">
<div><br></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + SmallVector<char, 20> Buf;<br>
> + if (ScopeName)<br>
> + AttrName = (ScopeName->getName() + "___" + AttrName).toStringRef(Buf);<br>
> +<br>
<br>
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.<br></blockquote>
</div><div>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 ;).</div><div class="im"><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + typedef std::vector<const Stmt*> StmtVec;<br>
> + bool GetFallThroughSourceStmts(const CFGBlock &B, StmtVec &Unannotated,<br>
> + StmtVec &Annotated) {<br>
<br>
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.<br>
<br>
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.<br></blockquote></div><div>I changed this to return just counts of annotated and unannotated fall-through source locations</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + SourceLocation L = Label->getLocStart();<br>
> + S.Diag(L, diag::note_insert_fallthrough_fixit) <<<br>
> + FixItHint::CreateInsertion(L, "[[clang::fallthrough]];");<br>
<br>
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?</blockquote>
</div><div>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.</div><div>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.</div>
<span class="HOEnZb"><font color="#888888">
<div><br></div></font></span></div><span class="HOEnZb"><font color="#888888">-- <br>
</font></span></div><span class="HOEnZb"><font color="#888888"><div>Best regards,</div><div>Alexander Kornienko</div><div><br></div>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div><div><font color="#666666"><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(213,15,37);border-right-color:rgb(213,15,37);border-bottom-color:rgb(213,15,37);border-left-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Alexander Kornienko |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(51,105,232);border-right-color:rgb(51,105,232);border-bottom-color:rgb(51,105,232);border-left-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span></font><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(0,153,57);border-right-color:rgb(0,153,57);border-bottom-color:rgb(0,153,57);border-left-color:rgb(0,153,57);padding-top:2px;margin-top:2px"><font color="#666666"> </font><a href="mailto:alexfh@google.com" style="color:rgb(17,85,204)" target="_blank">alexfh@google.com</a> |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(238,178,17);border-right-color:rgb(238,178,17);border-bottom-color:rgb(238,178,17);border-left-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> <a value="+35315435283" style="color:rgb(17,85,204)">+49 151 221 77 957</a></span></div>
</div><div><font color="#666666"><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Google Germany GmbH | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Dienerstr. 12 | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">80331 München</span></font></div>
<div><div style="font-family:Arial,Verdana,sans-serif;font-size:13px;background-color:rgb(255,255,255)"><font color="#666666"><font><br>AG Hamburg, HRB 86891 | Sitz der Gesellschaft: </font><font face="arial, sans-serif">Hamburg</font><font><br>
Geschäftsführer: Graham Law, Katherine Stephens<span style="border-collapse:separate"><span style="font-family:inherit"><br></span></span></font></font></div><div style="font-family:Arial,Verdana,sans-serif;font-size:13px;background-color:rgb(255,255,255)">
<font color="#666666"><br></font></div><div style="font-family:Arial,Verdana,sans-serif;font-size:13px;background-color:rgb(255,255,255)"><font><span style="border-collapse:collapse"><font face="arial, sans-serif" color="#666666">Tax ID:- 48/725/00206 </font></span></font></div>
<div style="font-family:Arial,Verdana,sans-serif;font-size:13px;background-color:rgb(255,255,255)"><font><span style="border-collapse:collapse"><font face="arial, sans-serif" color="#666666">VAT ID:- DE813741370</font></span></font></div>
</div><br>