gromer created this revision.
gromer added a reviewer: rsmith.
Herald added subscribers: cfe-commits, modocache.
Split CheckFallThroughForBody into separate implementations for blocks,
lambdas, coroutines, and all other functions. This simplifies the code because
virtually every part of it varies depending on what kind of function is being
processed; it's both clearer and safer to branch once at the beginning, rather
than branch repeatedly (sometimes on subtly different conditions) and/or rely
on layers of indirection. Notice, for example, how this refactoring surfaces
the inconsistency between the fast and slow paths for coroutines, and enables
the fast-path condition for functions to take advantage of more information
about what kinds of diagnostics the slow path can produce (see specifically how
SuggestNoReturn now feeds into the early-return condition).
Repository:
rC Clang
https://reviews.llvm.org/D51812
Files:
b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp
Index: b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -497,205 +497,221 @@
return AlwaysFallThrough;
}
-namespace {
+static void CheckFallThroughForBlock(Sema &S, const Stmt *Body,
+ const BlockExpr *blkExpr,
+ AnalysisDeclContext &AC) {
+ const FunctionType *FT =
+ blkExpr->getType()->getPointeeType()->getAs<FunctionType>();
+ if (FT == nullptr) return;
-struct CheckFallThroughDiagnostics {
- unsigned diag_MaybeFallThrough_HasNoReturn;
- unsigned diag_MaybeFallThrough_ReturnsNonVoid;
- unsigned diag_AlwaysFallThrough_HasNoReturn;
- unsigned diag_AlwaysFallThrough_ReturnsNonVoid;
- unsigned diag_NeverFallThroughOrReturn;
- enum { Function, Block, Lambda, Coroutine } funMode;
- SourceLocation FuncLoc;
-
- static CheckFallThroughDiagnostics MakeForFunction(const Decl *Func) {
- CheckFallThroughDiagnostics D;
- D.FuncLoc = Func->getLocation();
- D.diag_MaybeFallThrough_HasNoReturn =
- diag::warn_falloff_noreturn_function;
- D.diag_MaybeFallThrough_ReturnsNonVoid =
- diag::warn_maybe_falloff_nonvoid_function;
- D.diag_AlwaysFallThrough_HasNoReturn =
- diag::warn_falloff_noreturn_function;
- D.diag_AlwaysFallThrough_ReturnsNonVoid =
- diag::warn_falloff_nonvoid_function;
+ bool ReturnsValue = !FT->getReturnType()->isVoidType();
+ bool HasNoReturnAttr = FT->getNoReturnAttr();
- // Don't suggest that virtual functions be marked "noreturn", since they
- // might be overridden by non-noreturn functions.
- bool isVirtualMethod = false;
- if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(Func))
- isVirtualMethod = Method->isVirtual();
+ // Short circuit for compilation speed.
+ if (!ReturnsValue && !HasNoReturnAttr)
+ return;
- // Don't suggest that template instantiations be marked "noreturn"
- bool isTemplateInstantiation = false;
- if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(Func))
- isTemplateInstantiation = Function->isTemplateInstantiation();
+ SourceLocation RBrace = Body->getEndLoc();
+ switch (CheckFallThrough(AC)) {
+ case MaybeFallThrough:
+ if (HasNoReturnAttr)
+ S.Diag(RBrace, diag::err_noreturn_block_has_return_expr);
+ else if (ReturnsValue)
+ S.Diag(RBrace, diag::err_maybe_falloff_nonvoid_block);
+ break;
- if (!isVirtualMethod && !isTemplateInstantiation)
- D.diag_NeverFallThroughOrReturn =
- diag::warn_suggest_noreturn_function;
- else
- D.diag_NeverFallThroughOrReturn = 0;
+ case AlwaysFallThrough:
+ if (HasNoReturnAttr)
+ S.Diag(RBrace, diag::err_noreturn_block_has_return_expr);
+ else if (ReturnsValue)
+ S.Diag(RBrace, diag::err_falloff_nonvoid_block);
+ break;
- D.funMode = Function;
- return D;
+ case NeverFallThroughOrReturn:
+ case NeverFallThrough:
+ case UnknownFallThrough:
+ break;
}
+}
- static CheckFallThroughDiagnostics MakeForCoroutine(const Decl *Func) {
- CheckFallThroughDiagnostics D;
- D.FuncLoc = Func->getLocation();
- D.diag_MaybeFallThrough_HasNoReturn = 0;
- D.diag_MaybeFallThrough_ReturnsNonVoid =
- diag::warn_maybe_falloff_nonvoid_coroutine;
- D.diag_AlwaysFallThrough_HasNoReturn = 0;
- D.diag_AlwaysFallThrough_ReturnsNonVoid =
- diag::warn_falloff_nonvoid_coroutine;
- D.funMode = Coroutine;
- return D;
- }
-
- static CheckFallThroughDiagnostics MakeForBlock() {
- CheckFallThroughDiagnostics D;
- D.diag_MaybeFallThrough_HasNoReturn =
- diag::err_noreturn_block_has_return_expr;
- D.diag_MaybeFallThrough_ReturnsNonVoid =
- diag::err_maybe_falloff_nonvoid_block;
- D.diag_AlwaysFallThrough_HasNoReturn =
- diag::err_noreturn_block_has_return_expr;
- D.diag_AlwaysFallThrough_ReturnsNonVoid =
- diag::err_falloff_nonvoid_block;
- D.diag_NeverFallThroughOrReturn = 0;
- D.funMode = Block;
- return D;
- }
-
- static CheckFallThroughDiagnostics MakeForLambda() {
- CheckFallThroughDiagnostics D;
- D.diag_MaybeFallThrough_HasNoReturn =
- diag::err_noreturn_lambda_has_return_expr;
- D.diag_MaybeFallThrough_ReturnsNonVoid =
- diag::warn_maybe_falloff_nonvoid_lambda;
- D.diag_AlwaysFallThrough_HasNoReturn =
- diag::err_noreturn_lambda_has_return_expr;
- D.diag_AlwaysFallThrough_ReturnsNonVoid =
- diag::warn_falloff_nonvoid_lambda;
- D.diag_NeverFallThroughOrReturn = 0;
- D.funMode = Lambda;
- return D;
- }
-
- bool checkDiagnostics(DiagnosticsEngine &D, bool ReturnsVoid,
- bool HasNoReturn) const {
- if (funMode == Function) {
- return (ReturnsVoid ||
- D.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
- FuncLoc)) &&
- (!HasNoReturn ||
- D.isIgnored(diag::warn_noreturn_function_has_return_expr,
- FuncLoc)) &&
- (!ReturnsVoid ||
- D.isIgnored(diag::warn_suggest_noreturn_block, FuncLoc));
- }
- if (funMode == Coroutine) {
- return (ReturnsVoid ||
- D.isIgnored(diag::warn_maybe_falloff_nonvoid_function, FuncLoc) ||
- D.isIgnored(diag::warn_maybe_falloff_nonvoid_coroutine,
- FuncLoc)) &&
- (!HasNoReturn);
- }
- // For blocks / lambdas.
- return ReturnsVoid && !HasNoReturn;
+static void CheckFallThroughForLambda(Sema &S, const Decl *D, const Stmt *Body,
+ AnalysisDeclContext &AC) {
+ const auto *FD = dyn_cast<FunctionDecl>(D);
+ if (FD == nullptr) return;
+
+ // cpu_dispatch functions permit empty function bodies for ICC compatibility.
+ if (FD->isCPUDispatchMultiVersion())
+ return;
+
+ bool ReturnsValue = !FD->getReturnType()->isVoidType();
+ bool HasNoReturnAttr = FD->isNoReturn();
+
+ // Short circuit for compilation speed.
+ if (!ReturnsValue && !HasNoReturnAttr)
+ return;
+
+ SourceLocation RBrace = Body->getEndLoc();
+ switch (CheckFallThrough(AC)) {
+ case MaybeFallThrough:
+ if (HasNoReturnAttr)
+ S.Diag(RBrace, diag::err_noreturn_lambda_has_return_expr);
+ else if (ReturnsValue)
+ S.Diag(RBrace, diag::warn_maybe_falloff_nonvoid_lambda);
+ break;
+ case AlwaysFallThrough:
+ if (HasNoReturnAttr)
+ S.Diag(RBrace, diag::err_noreturn_lambda_has_return_expr);
+ else if (ReturnsValue)
+ S.Diag(RBrace, diag::warn_falloff_nonvoid_lambda);
+ break;
+ case NeverFallThroughOrReturn:
+ case NeverFallThrough:
+ case UnknownFallThrough:
+ break;
}
-};
+}
-} // anonymous namespace
+static void CheckFallThroughForCoroutine(
+ Sema &S, const Decl *D, const Stmt *Body,
+ AnalysisDeclContext &AC, sema::FunctionScopeInfo *FSI) {
+ const auto *FD = dyn_cast<FunctionDecl>(D);
+ if (FD == nullptr) return;
+
+ const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body);
+ bool ReturnsValue = CBody ? (CBody->getFallthroughHandler() == nullptr)
+ : !FD->getReturnType()->isVoidType();
+ bool HasNoReturnAttr = FD->isNoReturn();
-/// CheckFallThroughForBody - Check that we don't fall off the end of a
-/// function that should return a value. Check that we don't fall off the end
-/// of a noreturn function. We assume that functions and blocks not marked
-/// noreturn will return.
-static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
- const BlockExpr *blkExpr,
- const CheckFallThroughDiagnostics &CD,
- AnalysisDeclContext &AC,
- sema::FunctionScopeInfo *FSI) {
+ // Short circuit for compilation speed.
+ //
+ // FIXME this logic does not appear to correspond to the logic on the
+ // slow path.
+ DiagnosticsEngine &Diags = S.getDiagnostics();
+ if ((!ReturnsValue ||
+ Diags.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
+ D->getLocation()) ||
+ Diags.isIgnored(diag::warn_maybe_falloff_nonvoid_coroutine,
+ D->getLocation())) &&
+ (!HasNoReturnAttr))
+ return;
+ SourceLocation RBrace = Body->getEndLoc();
+ switch (CheckFallThrough(AC)) {
+ case MaybeFallThrough:
+ if (ReturnsValue)
+ S.Diag(RBrace, diag::warn_maybe_falloff_nonvoid_coroutine)
+ << FSI->CoroutinePromise->getType();
+ break;
+ case AlwaysFallThrough:
+ if (ReturnsValue)
+ S.Diag(RBrace, diag::warn_falloff_nonvoid_coroutine)
+ << FSI->CoroutinePromise->getType();
+ break;
+ case NeverFallThroughOrReturn:
+ case NeverFallThrough:
+ case UnknownFallThrough:
+ break;
+ }
+}
+
+static void CheckFallThroughForFunction(
+ Sema &S, const Decl *D, const Stmt *Body,
+ AnalysisDeclContext &AC) {
bool ReturnsVoid = false;
- bool HasNoReturn = false;
- bool IsCoroutine = FSI->isCoroutine();
+ bool HasNoReturnAttr = false;
+ bool SuggestNoReturn = true;
if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
- if (const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body))
- ReturnsVoid = CBody->getFallthroughHandler() != nullptr;
- else
- ReturnsVoid = FD->getReturnType()->isVoidType();
- HasNoReturn = FD->isNoReturn();
+ ReturnsVoid = FD->getReturnType()->isVoidType();
+ HasNoReturnAttr = FD->isNoReturn();
+
+ // Don't suggest that virtual functions be marked "noreturn", since they
+ // might be overridden by non-noreturn functions.
+ bool isVirtualMethod = false;
+ if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(FD))
+ isVirtualMethod = Method->isVirtual();
+
+ // Don't suggest that template instantiations be marked "noreturn"
+ bool isTemplateInstantiation = false;
+ if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(FD))
+ isTemplateInstantiation = Function->isTemplateInstantiation();
+
+ SuggestNoReturn = !isVirtualMethod && !isTemplateInstantiation;
}
else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
ReturnsVoid = MD->getReturnType()->isVoidType();
- HasNoReturn = MD->hasAttr<NoReturnAttr>();
- }
- else if (isa<BlockDecl>(D)) {
- QualType BlockTy = blkExpr->getType();
- if (const FunctionType *FT =
- BlockTy->getPointeeType()->getAs<FunctionType>()) {
- if (FT->getReturnType()->isVoidType())
- ReturnsVoid = true;
- if (FT->getNoReturnAttr())
- HasNoReturn = true;
- }
+ HasNoReturnAttr = MD->hasAttr<NoReturnAttr>();
}
- DiagnosticsEngine &Diags = S.getDiagnostics();
-
- // Short circuit for compilation speed.
- if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn))
- return;
- SourceLocation LBrace = Body->getBeginLoc(), RBrace = Body->getEndLoc();
- auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) {
- if (IsCoroutine)
- S.Diag(Loc, DiagID) << FSI->CoroutinePromise->getType();
- else
- S.Diag(Loc, DiagID);
- };
-
// cpu_dispatch functions permit empty function bodies for ICC compatibility.
if (D->getAsFunction() && D->getAsFunction()->isCPUDispatchMultiVersion())
return;
+ // Short circuit for compilation speed.
+ DiagnosticsEngine &Diags = S.getDiagnostics();
+ if ((ReturnsVoid ||
+ Diags.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
+ D->getLocation())) &&
+ (!HasNoReturnAttr ||
+ Diags.isIgnored(diag::warn_noreturn_function_has_return_expr,
+ D->getLocation())) &&
+ (!ReturnsVoid || !SuggestNoReturn ||
+ Diags.isIgnored(diag::warn_suggest_noreturn_block, D->getLocation())))
+ return;
+
// Either in a function body compound statement, or a function-try-block.
+ SourceLocation LBrace = Body->getBeginLoc(), RBrace = Body->getEndLoc();
switch (CheckFallThrough(AC)) {
- case UnknownFallThrough:
- break;
-
case MaybeFallThrough:
- if (HasNoReturn)
- EmitDiag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
+ if (HasNoReturnAttr)
+ S.Diag(RBrace, diag::warn_falloff_noreturn_function);
else if (!ReturnsVoid)
- EmitDiag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
+ S.Diag(RBrace, diag::warn_maybe_falloff_nonvoid_function);
break;
case AlwaysFallThrough:
- if (HasNoReturn)
- EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
+ if (HasNoReturnAttr)
+ S.Diag(RBrace, diag::warn_falloff_noreturn_function);
else if (!ReturnsVoid)
- EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
+ S.Diag(RBrace, diag::warn_falloff_nonvoid_function);
break;
case NeverFallThroughOrReturn:
- if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {
+ if (ReturnsVoid && !HasNoReturnAttr && SuggestNoReturn) {
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
- S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 0 << FD;
+ S.Diag(LBrace, diag::warn_suggest_noreturn_function) << 0 << FD;
} else if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
- S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 1 << MD;
+ S.Diag(LBrace, diag::warn_suggest_noreturn_function) << 1 << MD;
} else {
- S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn);
+ S.Diag(LBrace, diag::warn_suggest_noreturn_function);
}
}
break;
case NeverFallThrough:
+ case UnknownFallThrough:
break;
}
}
+/// CheckFallThroughForBody - Check that we don't fall off the end of a
+/// function that should return a value. Check that we don't fall off the end
+/// of a noreturn function. We assume that functions and blocks not marked
+/// noreturn will return.
+static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
+ const BlockExpr *blkExpr,
+ AnalysisDeclContext &AC,
+ sema::FunctionScopeInfo *FSI) {
+ if (isa<BlockDecl>(D)) {
+ CheckFallThroughForBlock(S, Body, blkExpr, AC);
+ } else if (isa<CXXMethodDecl>(D) &&
+ cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call &&
+ cast<CXXMethodDecl>(D)->getParent()->isLambda()) {
+ CheckFallThroughForLambda(S, D, Body, AC);
+ } else if (FSI->isCoroutine()) {
+ CheckFallThroughForCoroutine(S, D, Body, AC, FSI);
+ } else {
+ CheckFallThroughForFunction(S, D, Body, AC);
+ }
+}
+
//===----------------------------------------------------------------------===//
// -Wuninitialized
//===----------------------------------------------------------------------===//
@@ -2113,17 +2129,7 @@
// Warning: check missing 'return'
if (P.enableCheckFallThrough) {
- const CheckFallThroughDiagnostics &CD =
- (isa<BlockDecl>(D)
- ? CheckFallThroughDiagnostics::MakeForBlock()
- : (isa<CXXMethodDecl>(D) &&
- cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call &&
- cast<CXXMethodDecl>(D)->getParent()->isLambda())
- ? CheckFallThroughDiagnostics::MakeForLambda()
- : (fscope->isCoroutine()
- ? CheckFallThroughDiagnostics::MakeForCoroutine(D)
- : CheckFallThroughDiagnostics::MakeForFunction(D)));
- CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC, fscope);
+ CheckFallThroughForBody(S, D, Body, blkExpr, AC, fscope);
}
// Warning: check for unreachable code
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
