diff --git a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h --- a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h +++ b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h @@ -23,12 +23,15 @@ ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context); void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: const bool WarnOnUnfixable; const bool WarnOnConditionVariables; + SmallVector PPConditionals; }; } // namespace readability diff --git a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp @@ -10,6 +10,7 @@ #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" +#include "clang/Lex/Preprocessor.h" #include "clang/Tooling/FixIt.h" #include "llvm/ADT/SmallVector.h" @@ -19,10 +20,23 @@ namespace tidy { namespace readability { -static const char ReturnStr[] = "return"; -static const char ContinueStr[] = "continue"; -static const char BreakStr[] = "break"; -static const char ThrowStr[] = "throw"; +namespace { + +class PPConditionalCollector : public PPCallbacks { +public: + PPConditionalCollector(SmallVectorImpl &Collection) + : Collection(Collection) {} + void Endif(SourceLocation Loc, SourceLocation IfLoc) override { + Collection.emplace_back(IfLoc, Loc); + } + +private: + SmallVectorImpl &Collection; +}; + +} // namespace + +static const char InterruptingStr[] = "interrupting"; static const char WarningMessage[] = "do not use 'else' after '%0'"; static const char WarnOnUnfixableStr[] = "WarnOnUnfixable"; static const char WarnOnConditionVariablesStr[] = "WarnOnConditionVariables"; @@ -94,7 +108,7 @@ } static void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context, - const Stmt *Else, SourceLocation ElseLoc) { + const Stmt *Else, SourceLocation ElseLoc) { auto Remap = [&](SourceLocation Loc) { return Context.getSourceManager().getExpansionLoc(Loc); }; @@ -140,11 +154,18 @@ Options.store(Opts, WarnOnConditionVariablesStr, WarnOnConditionVariables); } +void ElseAfterReturnCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks( + std::make_unique(this->PPConditionals)); +} + void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) { - const auto InterruptsControlFlow = - stmt(anyOf(returnStmt().bind(ReturnStr), continueStmt().bind(ContinueStr), - breakStmt().bind(BreakStr), - expr(ignoringImplicit(cxxThrowExpr().bind(ThrowStr))))); + const auto InterruptsControlFlow = stmt(anyOf( + returnStmt().bind(InterruptingStr), continueStmt().bind(InterruptingStr), + breakStmt().bind(InterruptingStr), + expr(ignoringImplicit(cxxThrowExpr().bind(InterruptingStr))))); Finder->addMatcher( compoundStmt( forEach(ifStmt(unless(isConstexpr()), @@ -157,21 +178,55 @@ this); } +static bool hasPreprocessorBranchEndBetweenLocations( + ArrayRef ConditionalBranchLocations, + SourceLocation ExpandedLocStart, SourceLocation ExpandedLocEnd) { + assert(ExpandedLocStart < ExpandedLocEnd); + + // First conditional block that ends after ExpandedLocStart + const auto *Begin = + llvm::lower_bound(ConditionalBranchLocations, ExpandedLocStart, + [](const SourceRange &LHS, const SourceLocation &RHS) { + return LHS.getEnd() < RHS; + }); + const auto *End = ConditionalBranchLocations.end(); + for (; Begin != End && Begin->getEnd() < ExpandedLocEnd; ++Begin) + if (Begin->getBegin() < ExpandedLocStart) + return true; + return false; +} + +static StringRef getControlFlowString(const Stmt &Stmt) { + if (isa(Stmt)) + return "return"; + if (isa(Stmt)) + return "continue"; + if (isa(Stmt)) + return "break"; + if (isa(Stmt)) + return "throw"; + llvm_unreachable("Unknown control flow interruptor"); +} + void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) { + assert(llvm::is_sorted(PPConditionals, + [](const SourceRange &LHS, const SourceRange &RHS) { + return LHS.getEnd() < RHS.getEnd(); + })); const auto *If = Result.Nodes.getNodeAs("if"); const auto *Else = Result.Nodes.getNodeAs("else"); const auto *OuterScope = Result.Nodes.getNodeAs("cs"); - - bool IsLastInScope = OuterScope->body_back() == If; + const auto *Interrupt = Result.Nodes.getNodeAs(InterruptingStr); SourceLocation ElseLoc = If->getElseLoc(); - auto ControlFlowInterruptor = [&]() -> llvm::StringRef { - for (llvm::StringRef BindingName : - {ReturnStr, ContinueStr, BreakStr, ThrowStr}) - if (Result.Nodes.getNodeAs(BindingName)) - return BindingName; - return {}; - }(); + if (hasPreprocessorBranchEndBetweenLocations( + PPConditionals, + Result.SourceManager->getExpansionLoc(Interrupt->getBeginLoc()), + Result.SourceManager->getExpansionLoc(ElseLoc))) + return; + + bool IsLastInScope = OuterScope->body_back() == If; + StringRef ControlFlowInterruptor = getControlFlowString(*Interrupt); if (!IsLastInScope && containsDeclInScope(Else)) { if (WarnOnUnfixable) { diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp @@ -1,5 +1,4 @@ // RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions -std=c++17 - namespace std { struct string { string(const char *); @@ -226,3 +225,89 @@ } return; } + +void testPPConditionals() { + + // These cases the return isn't inside the conditional so diagnose as normal. + if (true) { + return; +#if 1 +#endif + } else { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + return; + } + if (true) { +#if 1 +#endif + return; + } else { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + return; + } + + // No return here in the AST, no special handling needed. + if (true) { +#if 0 + return; +#endif + } else { + return; + } + + // Return here is inside a preprocessor conditional block, ignore this case. + if (true) { +#if 1 + return; +#endif + } else { + return; + } + + // These cases, Same as above but with an #else block + if (true) { +#if 1 + return; +#else +#endif + } else { + return; + } + if (true) { +#if 0 +#else + return; +#endif + } else { + return; + } + +// ensure it can handle macros +#define RETURN return + if (true) { +#if 1 + RETURN; +#endif + } else { + return; + } +#define ELSE else + if (true) { +#if 1 + return; +#endif + } + ELSE { + return; + } + + // Whole statement is in a conditional block so diagnost as normal. +#if 1 + if (true) { + return; + } else { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return' + return; + } +#endif +}