njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
njames93 requested review of this revision.
Consider this code:
if (Cond) {
#ifdef X_SUPPORTED
X();
#else
return;
#endif
} else {
Y();
}
Z();
In this example, if `X_SUPPORTED` is not defined, currently we'll get a warning
from the else-after-return check. However If we apply that fix, and then the
code is recompiled with `X_SUPPORTED` defined, we have inadvertently changed
the behaviour of the if statement due to the else being removed. Code flow when
`Cond` is `true` will be:
X();
Y();
Z();
where as before the fix it was:
X();
Z();
This patch adds checks that guard against `#endif` directives appearing between
the control flow interrupter and the else and not applying the fix if they are
detected.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91485
Files:
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
+++ 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
+}
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ 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<SourceRange, 0> PPConditionals;
};
} // namespace readability
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ 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<SourceRange> &Collection)
+ : Collection(Collection) {}
+ void Endif(SourceLocation Loc, SourceLocation IfLoc) override {
+ Collection.emplace_back(IfLoc, Loc);
+ }
+
+private:
+ SmallVectorImpl<SourceRange> &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<PPConditionalCollector>(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<SourceRange> 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<ReturnStmt>(Stmt))
+ return "return";
+ if (isa<ContinueStmt>(Stmt))
+ return "continue";
+ if (isa<BreakStmt>(Stmt))
+ return "break";
+ if (isa<CXXThrowExpr>(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<IfStmt>("if");
const auto *Else = Result.Nodes.getNodeAs<Stmt>("else");
const auto *OuterScope = Result.Nodes.getNodeAs<CompoundStmt>("cs");
-
- bool IsLastInScope = OuterScope->body_back() == If;
+ const auto *Interrupt = Result.Nodes.getNodeAs<Stmt>(InterruptingStr);
SourceLocation ElseLoc = If->getElseLoc();
- auto ControlFlowInterruptor = [&]() -> llvm::StringRef {
- for (llvm::StringRef BindingName :
- {ReturnStr, ContinueStr, BreakStr, ThrowStr})
- if (Result.Nodes.getNodeAs<Stmt>(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) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
