[cfe-commits] [PATCH] Improve diagnostic for -Wmissing-noreturn

[Chandler Carruth]

On Sat, Aug 27, 2011 at 4:00 PM, Joerg Sonnenberger <joerg at britannica.bec.de
> wrote:

> Hi all,
> attached patch tries to make the -Wmissing-noreturn message a bit more
> useful. Often enough, the function in question has the prototype in a
> separate file, so it avoids work knowing the name in advance. I'm not
> sure whether it is better to print the plain name or the full diagnostic
> string, so I'm going with the latter for now.
>

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.

A couple of comments on the patch:

Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 138705)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -236,7 +236,7 @@
 def err_falloff_nonvoid_block : Error<
   "control reaches end of non-void block">;
 def warn_suggest_noreturn_function : Warning<
-  "function could be attribute 'noreturn'">,
+  "function '%0' could be attribute 'noreturn'">,

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. ;]

   InGroup<DiagGroup<"missing-noreturn">>, DefaultIgnore;
 def warn_suggest_noreturn_block : Warning<
   "block could be attribute 'noreturn'">,
Index: lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- lib/Sema/AnalysisBasedWarnings.cpp (revision 138705)
+++ lib/Sema/AnalysisBasedWarnings.cpp (working copy)
@@ -376,9 +376,17 @@
                  CD.diag_AlwaysFallThrough_ReturnsNonVoid);
         break;
       case NeverFallThroughOrReturn:
-        if (ReturnsVoid && !HasNoReturn &&
CD.diag_NeverFallThroughOrReturn)
-          S.Diag(Compound->getLBracLoc(),
-                 CD.diag_NeverFallThroughOrReturn);
+        if (ReturnsVoid && !HasNoReturn &&
CD.diag_NeverFallThroughOrReturn) {
+          if (const FunctionDecl *FC = dyn_cast<FunctionDecl>(D)) {

FC? not FD?

+            std::string str;
+            FC->getNameForDiagnostic(str, S.Context.PrintingPolicy, true);

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?

+            S.Diag(Compound->getLBracLoc(),
+                   CD.diag_NeverFallThroughOrReturn) << str;
+          } else {
+            S.Diag(Compound->getLBracLoc(),
+                   CD.diag_NeverFallThroughOrReturn);

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