Page MenuHomePhabricator
Differential D130181 Diff 449048 clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

Changeset View

Standalone View

View Options

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

  • This file was added.
//===--- UseEarlyExitsCheck.cpp - clang-tidy ------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
#include "UseEarlyExitsCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Expr.h"
#include "clang/AST/OperationKinds.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
namespace clang {
namespace tidy {
namespace readability {
static bool needsParensAfterUnaryNegation(const Expr *E) {
E = E->IgnoreImpCasts();
if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
return true;
if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
Op->getOperator() != OO_Subscript;
return false;
}
static void addReverseConditionFix(DiagnosticBuilder &Diag,
const Expr *Condition,
const ASTContext &Ctx) {
if (const auto *BO = dyn_cast<BinaryOperator>(Condition)) {
if (BO->isComparisonOp() && BO->getOpcode() != BO_Cmp) {
Diag << FixItHint::CreateReplacement(
BO->getOperatorLoc(),
BinaryOperator::getOpcodeStr(
BinaryOperator::negateComparisonOp(BO->getOpcode())));
return;
}
} else if (const auto *UO = dyn_cast<UnaryOperator>(Condition)) {
if (UO->getOpcode() == UO_LNot) {
Diag << FixItHint::CreateRemoval(UO->getOperatorLoc());
if (const auto *Parens = dyn_cast<ParenExpr>(UO->getSubExpr())) {
Diag << FixItHint::CreateRemoval(Parens->getLParen());
Diag << FixItHint::CreateRemoval(Parens->getRParen());
}
return;
}
} else if (const auto *BL = dyn_cast<CXXBoolLiteralExpr>(Condition)) {
Diag << FixItHint::CreateReplacement(BL->getSourceRange(),
BL->getValue() ? "false" : "true");
return;
} else if (const auto *IL = dyn_cast<IntegerLiteral>(Condition)) {
Diag << FixItHint::CreateReplacement(
IL->getSourceRange(), IL->getValue().getBoolValue() ? "0" : "1");
return;
}
if (needsParensAfterUnaryNegation(Condition)) {
Diag << FixItHint::CreateInsertion(Condition->getBeginLoc(), "!(")
<< FixItHint::CreateInsertion(
JonasTothUnsubmitted
Done
ReplyInline Actions

did you consider comma expressions?

if (myDirtyCode(), myCondition && yourCondition). It seems to me, that the transformation would be incorrect.

JonasToth: did you consider comma expressions? `if (myDirtyCode(), myCondition && yourCondition)`. It…
njames93AuthorUnsubmitted
Done
ReplyInline Actions

Comma operator is a binary operator, so the transformation would wrap the whole expression in parens.

njames93: Comma operator is a binary operator, so the transformation would wrap the whole expression in…
Lexer::getLocForEndOfToken(Condition->getEndLoc(), 0,
Ctx.getSourceManager(),
Ctx.getLangOpts()),
")");
} else {
Diag << FixItHint::CreateInsertion(Condition->getBeginLoc(), "!");
}
}
static void addConjunctionReverseOneStep(DiagnosticBuilder &Diag, const Expr *E,
const ASTContext &Ctx,
StringRef Replacement) {
const auto *BO = dyn_cast<BinaryOperator>(E);
if (BO && BO->getOpcode() == BO_LAnd) {
addConjunctionReverseOneStep(Diag, BO->getLHS(), Ctx, Replacement);
Diag << FixItHint::CreateReplacement(BO->getOperatorLoc(), Replacement);
addConjunctionReverseOneStep(Diag, BO->getRHS(), Ctx, Replacement);
} else {
addReverseConditionFix(Diag, E, Ctx);
}
}
static void addConjunctionReverseFix(DiagnosticBuilder &Diag,
const Expr *Condition,
const ASTContext &Ctx,
StringRef Continuation) {
const auto *BO = dyn_cast<BinaryOperator>(Condition);
if (!BO || BO->getOpcode() != BO_LAnd) {
addReverseConditionFix(Diag, Condition, Ctx);
return;
}
addConjunctionReverseOneStep(Diag, Condition, Ctx,
SmallString<32>({") ", Continuation, "\nif ("}));
}
static llvm::iterator_range<CompoundStmt::const_reverse_body_iterator>
getBodyReverse(const CompoundStmt *S) {
return {S->body_rbegin(), S->body_rend()};
}
namespace {
class EarlyExitVisitor : public RecursiveASTVisitor<EarlyExitVisitor> {
UseEarlyExitsCheck &Check;
ASTContext &Ctx;
public:
EarlyExitVisitor(UseEarlyExitsCheck &Check, ASTContext &Ctx)
: Check(Check), Ctx(Ctx) {}
bool traverse() { return TraverseAST(Ctx); }
template <typename TerminationStmt>
void diagnoseIf(const IfStmt *If, StringRef Continuation);
template <typename TerminationStmt>
void checkBody(const CompoundStmt *CS, StringRef Continuation) {
for (const Stmt *Item : getBodyReverse(CS)) {
// If the last statement in the function is a return, ignore it.
if (isa<TerminationStmt>(Item))
continue;
// While were here, we can safely ignore empty null stmts.
if (isa<NullStmt>(Item) && !cast<NullStmt>(Item)->hasLeadingEmptyMacro())
continue;
if (const auto *If = dyn_cast<IfStmt>(Item))
diagnoseIf<TerminationStmt>(If, Continuation);
break;
}
}
bool VisitFunctionDecl(const FunctionDecl *Func) {
// Skip any declarations.
if (!Func->doesThisDeclarationHaveABody())
return true;
// Just ignore no return functions.
if (Func->isNoReturn())
return true;
// Early exit logic only works for functions that return void.
// FIXME: Explore options where following the IfStmt there is a return
// value with a literal return.
// bool hasSomething(Class *S) {
// if (S) {
// return S->hasSomething();
// }
// return false;
// }
// bool hasSomething(Class *S) {
// if (!S)
// return false;
// return S->hasSomething();
// return false; // Ideally this would be removed too.
// }
if (!Func->getReturnType()->isVoidType())
return true;
// FIXME: explore if CoreturnStmt could work here also.
checkBody<ReturnStmt>(cast<CompoundStmt>(Func->getBody()), "return");
return true;
}
bool VisitForStmt(const ForStmt *For) {
if (const auto *CS = dyn_cast_or_null<CompoundStmt>(For->getBody())) {
checkBody<ContinueStmt>(CS, "continue");
}
return true;
}
bool VisitWhileStmt(const WhileStmt *While) {
if (const auto *CS = dyn_cast_or_null<CompoundStmt>(While->getBody())) {
checkBody<ContinueStmt>(CS, "continue");
}
return true;
}
bool VisitDoStmt(const DoStmt *Do) {
if (const auto *CS = dyn_cast_or_null<CompoundStmt>(Do->getBody())) {
checkBody<ContinueStmt>(CS, "continue");
}
return true;
}
bool VisitCXXForRangeStmt(const CXXForRangeStmt *ForRange) {
if (const auto *CS = dyn_cast_or_null<CompoundStmt>(ForRange->getBody())) {
checkBody<ContinueStmt>(CS, "continue");
}
return true;
}
};
template <typename TerminationStmt>
void EarlyExitVisitor::diagnoseIf(const IfStmt *If, StringRef Continuation) {
// Ignore const(expr|eval) if statements.
if (If->isConsteval() || If->isConstexpr())
return;
// We can't transform if there is an else.
if (If->hasElseStorage())
return;
// If there is a variable in the condition, This transformation would mean it
// goes out of scope before the current then branch can use it.
if (If->hasVarStorage())
return;
// As above, only technically if the init doesn't actually have any decls, we
// could still do the transformation. But thats not exactly a common idiom.
if (If->hasInitStorage())
return;
const auto *BodyCS = llvm::dyn_cast_or_null<CompoundStmt>(If->getThen());
if (!BodyCS)
return;
const SourceManager &SM = Ctx.getSourceManager();
SourceLocation Begin = If->getBeginLoc();
SourceLocation End = If->getEndLoc();
FileID BeginFile = SM.getFileID(Begin);
njames93AuthorUnsubmitted
Done
ReplyInline Actions

I have an idea that we could infer this using the readability-braces-around-statements and its hicpp alias.
If enabled, probe the ShortStatementLines option to infer whether or not that check would fire if we make this transformation without using braces.

njames93: I have an idea that we could infer this using the `readability-braces-around-statements` and…
FileID EndFile = SM.getFileID(End);
if (BeginFile != EndFile)
return;
unsigned BeginLine = SM.getSpellingLineNumber(Begin);
unsigned EndLine = SM.getSpellingLineNumber(End);
if (BeginLine > EndLine)
return; // This case can't be good.
if ((EndLine - BeginLine) < Check.getLineCountThreshold())
return;
{
SmallString<32> C2({(Check.getWrapInBraces() ? "{\n" : "\n"), Continuation,
Check.getWrapInBraces() ? ";\n}" : ";\n"});
auto Diag = Check.diag(If->getBeginLoc(), "use early exit")
<< FixItHint::CreateInsertion(If->getThen()->getBeginLoc(), C2)
<< FixItHint::CreateRemoval(BodyCS->getLBracLoc())
<< FixItHint::CreateRemoval(BodyCS->getRBracLoc());
if (Check.getSplitConjunctions())
addConjunctionReverseFix(Diag, If->getCond(), Ctx, C2);
else
addReverseConditionFix(Diag, If->getCond(), Ctx);
}
checkBody<TerminationStmt>(BodyCS, Continuation);
}
} // namespace
static bool fallbackBracesCheckActive(llvm::Optional<bool> Cur,
ClangTidyContext *Context) {
if (Cur)
return *Cur;
static constexpr size_t BSize = 64; // Should only ever need 63 chars here.
char Buff[BSize];
constexpr llvm::StringLiteral OptName = ".ShortStatementLines";
constexpr llvm::StringLiteral CheckAliases[] = {
"google-readability-braces-around-statements",
"readability-braces-around-statements", "hicpp-braces-around-statements"};
for (StringRef CheckAlias : CheckAliases) {
if (!Context->isCheckEnabled(CheckAlias))
continue;
assert(BSize >= CheckAlias.size() + OptName.size());
memcpy(Buff, CheckAlias.data(), CheckAlias.size());
memcpy(Buff + CheckAlias.size(), OptName.data(), OptName.size());
auto Iter = Context->getOptions().CheckOptions.find(
{Buff, CheckAlias.size() + OptName.size()});
if (Iter == Context->getOptions().CheckOptions.end())
return true;
unsigned Value;
if (StringRef(Iter->getValue().Value).getAsInteger(10, Value) || Value < 2)
return true;
}
return false;
}
UseEarlyExitsCheck::UseEarlyExitsCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
LineCountThreshold(Options.get("LineCountThreshold", 10U)),
SplitConjunctions(Options.get("SplitConjunctions", false)),
WrapInBraces(fallbackBracesCheckActive(Options.get<bool>("WrapInBraces"),
Context)) {}
void UseEarlyExitsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "LineCountThreshold", LineCountThreshold);
Options.store(Opts, "SplitConjunctions", SplitConjunctions);
Options.store(Opts, "WrapInBraces", WrapInBraces);
}
void UseEarlyExitsCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(translationUnitDecl(), this);
}
void UseEarlyExitsCheck::check(const MatchFinder::MatchResult &Result) {
EarlyExitVisitor(*this, *Result.Context).traverse();
}
} // namespace readability
} // namespace tidy
} // namespace clang
JonasTothUnsubmitted
Not Done
ReplyInline Actions

i would really prefer to just use strings here.
std::string Buff = CheckAlias.str() + OptName.str(); is much easier to understand and equivalent (?)

this function is called only once in the constructor as well, so speed and allocations are not valid in my opinion.

JonasToth: i would really prefer to just use strings here. `std::string Buff = CheckAlias.str() + OptName.
PiotrZSLUnsubmitted
Not Done
ReplyInline Actions

This will trigger on all system code, and then users will complain again that they see poor clang-tidy performance...

when it could be just something like

ifStmt(unless(isExpansionInSystemHeader()),
       unless(isConsteval()),
       unless(isConstexpr()),
       unless(hasElse(stmt())),
       unless(hasInitStatement(stmt()),
       hasThen(compoundStmt(hasSizeAboeLineTreshold())),
       ...

Simply everything that could be put into matcher should be put into matcher (easier to maintain), also what's a point of checking functions that doesn't have if's. On that point also some implicit functions or if statement should be ignored, to avoid checking templates twice.

PiotrZSL: This will trigger on all system code, and then users will complain again that they see poor…
njames93AuthorUnsubmitted
Done
ReplyInline Actions

Not triggering on system headers is a good point, but for raw performance, that code is better residing in the visitor, which can massively outperform matchers as we can straight up ignore huge blocks of code that aren't of interest.

njames93: Not triggering on system headers is a good point, but for raw performance, that code is better…