Hi Richard,<div><br></div><div>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.</div>
<div>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.</div><div>Thanks for your review!<br>
<br><div class="gmail_quote">On Wed, May 2, 2012 at 12:42 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">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 Richard,<div><br></div><div>Here's an updated patch.<br><br><div class="gmail_quote"><div class="im">On Tue, May 1, 2012 at 5:28 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="gmail_quote">Hi Alex,</div><div class="gmail_quote"><br>
</div>
<div class="gmail_quote">The 'unreachable block' test isn't quite right; a block can have (unreachable) predecessors and still be unreachable:</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">switch (x) {</div><div class="gmail_quote">// oops, deleted "case 0:"</div><div class="gmail_quote"> if (a)</div><div class="gmail_quote"> f();</div>
<div class="gmail_quote"> [[clang::fallthrough]]; // no warning here!</div><div class="gmail_quote">case 1:</div><div class="gmail_quote"> break;</div><div class="gmail_quote">}</div><div class="gmail_quote"><br></div>
<div class="gmail_quote">
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.</div>
</div></div></blockquote></div><div>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.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="gmail_quote">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.</div>
</div></div></blockquote></div><div>Done. The only question is: is naming style enforced somehow? RecursiveASTVisitor.h, for example, uses different conventions.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail_quote">On Mon, Apr 30, 2012 at 5:40 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:</div>
</div></div><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="gmail_quote"><div>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></div></blockquote><div><br></div></div><div>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.</div></div></div></blockquote>
</div><div>Added.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="gmail_quote"><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;</blockquote></div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + 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></div></blockquote><div><br></div></div><div>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.</div>
</div></div></blockquote></div><div>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.</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"><div class="gmail_extra"><div class="gmail_quote"><div>I'd also suggest renaming that function, perhaps to CheckFallThroughIntoBlock.</div>
</div></div></blockquote></div><div>You mean checkFallThroughIntoBlock? ;) Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra">
<div class="gmail_quote">
<div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="gmail_quote"><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>
</div></div></blockquote></div></div><br></div><div class="gmail_extra">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 :-)</div>
</blockquote></div><div>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.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra">Other than the superficial issues above, I think this is looking good.</div><div class="gmail_extra">
<br></div><div class="gmail_extra">Thanks!</div><span><font color="#888888"><div class="gmail_extra">
Richard</div></font></span></blockquote></div></div></div></blockquote><div><br></div><div>-- </div><div>Best regards,</div><div>Alexander Kornienko </div></div>
</div>