<div dir="ltr">Hey Aaron,<div><br></div><div>I went ahead and made the two patches with the tests, let me know how they look! Is there anything I need to do to close D2506?</div><div><a href="http://llvm-reviews.chandlerc.com/D2507">http://llvm-reviews.chandlerc.com/D2507</a><br>
</div><div><a href="http://llvm-reviews.chandlerc.com/D2508">http://llvm-reviews.chandlerc.com/D2508</a><br></div><div><br>Thanks.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 3, 2014 at 12:33 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Jan 3, 2014 at 12:25 PM, Michael Bao <<a href="mailto:mike.h.bao@gmail.com">mike.h.bao@gmail.com</a>> wrote:<br>
> In response to Bug 10642 in Bugzilla (<a href="http://llvm.org/bugs/show_bug.cgi?id=10642" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=10642</a>). I've checked this against Revision 198291.<br>
><br>
> # Removed the warning about a 'return' statement in a 'noreturn' function if the 'return' statement is unreachable.<br>
> # Warn users if they have a non-void return type on a 'noreturn' function.<br>
><br>
> This is my first patch, let me know if there's anything I need to do :) Thanks!<br>
<br>
</div>Thanks for your submission!<br>
<br>
You should reformat the patch to meet the coding guidelines:<br>
<a href="http://www.llvm.org/docs/CodingStandards.html" target="_blank">http://www.llvm.org/docs/CodingStandards.html</a> For instance, spaces<br>
instead of tabs, no braces around single-line compound statements,<br>
etc.<br>
<br>
Also, this should ideally be split into two patches since there are<br>
two different things going on here.<br>
<br>
You should have test cases that demonstrate the behavioral changes for<br>
both patches.<br>
<div class="im"><br>
> <a href="http://llvm-reviews.chandlerc.com/D2506" target="_blank">http://llvm-reviews.chandlerc.com/D2506</a><br>
><br>
> Files:<br>
> lib/Sema/SemaStmt.cpp<br>
> lib/Sema/SemaDecl.cpp<br>
> include/clang/Basic/DiagnosticSemaKinds.td<br>
><br>
> Index: lib/Sema/SemaStmt.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaStmt.cpp<br>
> +++ lib/Sema/SemaStmt.cpp<br>
> @@ -2776,9 +2776,10 @@<br>
> QualType RelatedRetType;<br>
> if (const FunctionDecl *FD = getCurFunctionDecl()) {<br>
> FnRetType = FD->getResultType();<br>
> - if (FD->isNoReturn())<br>
> - Diag(ReturnLoc, diag::warn_noreturn_function_has_return_expr)<br>
> - << FD->getDeclName();<br>
> + if (FD->isNoReturn()) {<br>
> + DiagRuntimeBehavior(ReturnLoc, RetValExp,<br>
> + PDiag(diag::warn_noreturn_function_has_return_expr) << FD->getDeclName());<br>
<br>
</div>You can leave off the getDeclName here -- the diagnostics engine<br>
understands anything that's a NamedDecl.<br>
<div class="im"><br>
> + }<br>
> } else if (ObjCMethodDecl *MD = getCurMethodDecl()) {<br>
> FnRetType = MD->getResultType();<br>
> if (MD->hasRelatedResultType() && MD->getClassInterface()) {<br>
> Index: lib/Sema/SemaDecl.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaDecl.cpp<br>
> +++ lib/Sema/SemaDecl.cpp<br>
> @@ -7392,6 +7392,10 @@<br>
> AddToScope = false;<br>
> }<br>
><br>
> + if (NewFD->isNoReturn() && !NewFD->getResultType()->isVoidType()) {<br>
> + Diag(NewFD->getLocation(), diag::warn_noreturn_function_has_nonvoid_type)<br>
> + << NewFD->getDeclName();<br>
<br>
</div>You can also leave off the getDeclName here.<br>
<div class="im"><br>
> + }<br>
> return NewFD;<br>
> }<br>
><br>
> Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticSemaKinds.td<br>
> +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
> @@ -6462,6 +6462,9 @@<br>
> def warn_falloff_noreturn_function : Warning<<br>
> "function declared 'noreturn' should not return">,<br>
> InGroup<InvalidNoreturn>;<br>
> +def warn_noreturn_function_has_nonvoid_type : Warning<<br>
> + "function %0 declared 'noreturn' should have not have a non-void return type">,<br>
> + InGroup<InvalidNoreturn>;<br>
<br>
</div>This may be a bit more of a nitpick than is strictly necessary, but<br>
'noreturn' is the C++11 spelling, and _Noreturn is the C11 spelling --<br>
we should probably be a bit more distinct with the diagnostic for<br>
clarity purposes.<br>
<div class="im"><br>
> def err_noreturn_block_has_return_expr : Error<<br>
> "block declared 'noreturn' should not return">;<br>
> def err_noreturn_missing_on_first_decl : Error<<br>
><br>
</div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div>