[PATCH] Warnings for NoReturn Functions

Aaron Ballman aaron at aaronballman.com
Fri Jan 3 09:33:43 PST 2014


On Fri, Jan 3, 2014 at 12:25 PM, Michael Bao <mike.h.bao at gmail.com> wrote:
> In response to Bug 10642 in Bugzilla (http://llvm.org/bugs/show_bug.cgi?id=10642). I've checked this against Revision 198291.
>
>   #  Removed the warning about a 'return' statement in a 'noreturn' function if the 'return' statement is unreachable.
>   #  Warn users if they have a non-void return type on a 'noreturn' function.
>
> This is my first patch, let me know if there's anything I need to do :) Thanks!

Thanks for your submission!

You should reformat the patch to meet the coding guidelines:
http://www.llvm.org/docs/CodingStandards.html  For instance, spaces
instead of tabs, no braces around single-line compound statements,
etc.

Also, this should ideally be split into two patches since there are
two different things going on here.

You should have test cases that demonstrate the behavioral changes for
both patches.

> http://llvm-reviews.chandlerc.com/D2506
>
> Files:
>   lib/Sema/SemaStmt.cpp
>   lib/Sema/SemaDecl.cpp
>   include/clang/Basic/DiagnosticSemaKinds.td
>
> Index: lib/Sema/SemaStmt.cpp
> ===================================================================
> --- lib/Sema/SemaStmt.cpp
> +++ lib/Sema/SemaStmt.cpp
> @@ -2776,9 +2776,10 @@
>    QualType RelatedRetType;
>    if (const FunctionDecl *FD = getCurFunctionDecl()) {
>      FnRetType = FD->getResultType();
> -    if (FD->isNoReturn())
> -      Diag(ReturnLoc, diag::warn_noreturn_function_has_return_expr)
> -        << FD->getDeclName();
> +    if (FD->isNoReturn()) {
> +                       DiagRuntimeBehavior(ReturnLoc, RetValExp,
> +                               PDiag(diag::warn_noreturn_function_has_return_expr) << FD->getDeclName());

You can leave off the getDeclName here -- the diagnostics engine
understands anything that's a NamedDecl.

> +               }
>    } else if (ObjCMethodDecl *MD = getCurMethodDecl()) {
>      FnRetType = MD->getResultType();
>      if (MD->hasRelatedResultType() && MD->getClassInterface()) {
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -7392,6 +7392,10 @@
>      AddToScope = false;
>    }
>
> +       if (NewFD->isNoReturn() && !NewFD->getResultType()->isVoidType()) {
> +               Diag(NewFD->getLocation(), diag::warn_noreturn_function_has_nonvoid_type)
> +                       << NewFD->getDeclName();

You can also leave off the getDeclName here.

> +       }
>    return NewFD;
>  }
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -6462,6 +6462,9 @@
>  def warn_falloff_noreturn_function : Warning<
>    "function declared 'noreturn' should not return">,
>    InGroup<InvalidNoreturn>;
> +def warn_noreturn_function_has_nonvoid_type : Warning<
> +       "function %0 declared 'noreturn' should have not have a non-void return type">,
> +       InGroup<InvalidNoreturn>;

This may be a bit more of a nitpick than is strictly necessary, but
'noreturn' is the C++11 spelling, and _Noreturn is the C11 spelling --
we should probably be a bit more distinct with the diagnostic for
clarity purposes.

>  def err_noreturn_block_has_return_expr : Error<
>    "block declared 'noreturn' should not return">;
>  def err_noreturn_missing_on_first_decl : Error<
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

Thanks!

~Aaron



More information about the cfe-commits mailing list