<div class="gmail_quote">On Sat, Aug 27, 2011 at 4:00 PM, Joerg Sonnenberger <span dir="ltr"><<a href="mailto:joerg@britannica.bec.de">joerg@britannica.bec.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Hi all,<br>
attached patch tries to make the -Wmissing-noreturn message a bit more<br>
useful. Often enough, the function in question has the prototype in a<br>
separate file, so it avoids work knowing the name in advance. I'm not<br>
sure whether it is better to print the plain name or the full diagnostic<br>
string, so I'm going with the latter for now.<br></blockquote><div><br></div><div>I note that there are no tests updated in this patch... if we're going to touch this warning, we should add some good test coverage for it.</div>
<div><br></div><div>A couple of comments on the patch:</div><div><br></div><div><div>Index: include/clang/Basic/DiagnosticSemaKinds.td</div><div>===================================================================</div><div>
--- include/clang/Basic/DiagnosticSemaKinds.td<span class="Apple-tab-span" style="white-space:pre">   </span>(revision 138705)</div><div>+++ include/clang/Basic/DiagnosticSemaKinds.td<span class="Apple-tab-span" style="white-space:pre">      </span>(working copy)</div>
<div>@@ -236,7 +236,7 @@</div><div> def err_falloff_nonvoid_block : Error<</div><div>   "control reaches end of non-void block">;</div><div> def warn_suggest_noreturn_function : Warning<</div><div>-  "function could be attribute 'noreturn'">,</div>
<div>+  "function '%0' could be attribute 'noreturn'">,</div><div><br></div><div>This wording is awkward. Maybe "could be marked with the 'noreturn' attribute"? "could be marked with attribute 'noreturn'"? Essentially, 'attribute' doesn't seem like a good verb here. ;]</div>
<div><br></div><div>   InGroup<DiagGroup<"missing-noreturn">>, DefaultIgnore;</div><div> def warn_suggest_noreturn_block : Warning<</div><div>   "block could be attribute 'noreturn'">,</div>
<div>Index: lib/Sema/AnalysisBasedWarnings.cpp</div><div>===================================================================</div><div>--- lib/Sema/AnalysisBasedWarnings.cpp<span class="Apple-tab-span" style="white-space:pre">      </span>(revision 138705)</div>
<div>+++ lib/Sema/AnalysisBasedWarnings.cpp<span class="Apple-tab-span" style="white-space:pre">        </span>(working copy)</div><div>@@ -376,9 +376,17 @@</div><div>                  CD.diag_AlwaysFallThrough_ReturnsNonVoid);</div>
<div>         break;</div><div>       case NeverFallThroughOrReturn:</div><div>-        if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn)</div><div>-          S.Diag(Compound->getLBracLoc(),</div>
<div>-                 CD.diag_NeverFallThroughOrReturn);</div><div>+        if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {</div><div>+          if (const FunctionDecl *FC = dyn_cast<FunctionDecl>(D)) {</div>
<div><br></div><div>FC? not FD?</div><div><br></div><div>+            std::string str;</div><div>+            FC->getNameForDiagnostic(str, S.Context.PrintingPolicy, true);</div><div><br></div><div>I'm surprised this is necessary. Naively I would expect simply "<< FC" below to end up calling this exact function for us. Does that not work?</div>
<div><br></div><div>+            S.Diag(Compound->getLBracLoc(),</div><div>+                   CD.diag_NeverFallThroughOrReturn) << str;</div><div>+          } else {</div><div>+            S.Diag(Compound->getLBracLoc(),</div>
<div>+                   CD.diag_NeverFallThroughOrReturn);</div><div><br></div><div>This will likely crash... we need to provide *something* for the name in all of the cases where we emit this diagnostic. Testing would be especially good here. =D</div>
</div></div>