-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add clang-tidy check to suggest replacement of conditional statement …
…with std::min/std::max (#77816) This pull request fixes #64914 where author suggests adding a readability check to propose the replacement of conditional statements with std::min/std::max for improved code readability. Additionally, reference is made to PyLint's similar checks: [consider-using-min-builtin](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/consider-using-min-builtin.html) and [consider-using-max-builtin](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/consider-using-max-builtin.html)
- Loading branch information
Showing
8 changed files
with
524 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
188 changes: 188 additions & 0 deletions
188
clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,188 @@ | ||
//===--- UseStdMinMaxCheck.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 "UseStdMinMaxCheck.h" | ||
#include "../utils/ASTUtils.h" | ||
#include "clang/AST/ASTContext.h" | ||
#include "clang/ASTMatchers/ASTMatchFinder.h" | ||
#include "clang/Lex/Preprocessor.h" | ||
|
||
using namespace clang::ast_matchers; | ||
|
||
namespace clang::tidy::readability { | ||
|
||
namespace { | ||
|
||
// Ignore if statements that are inside macros. | ||
AST_MATCHER(IfStmt, isIfInMacro) { | ||
return Node.getIfLoc().isMacroID() || Node.getEndLoc().isMacroID(); | ||
} | ||
|
||
} // namespace | ||
|
||
static const llvm::StringRef AlgorithmHeader("<algorithm>"); | ||
|
||
static bool minCondition(const BinaryOperator::Opcode Op, const Expr *CondLhs, | ||
const Expr *CondRhs, const Expr *AssignLhs, | ||
const Expr *AssignRhs, const ASTContext &Context) { | ||
if ((Op == BO_LT || Op == BO_LE) && | ||
(tidy::utils::areStatementsIdentical(CondLhs, AssignRhs, Context) && | ||
tidy::utils::areStatementsIdentical(CondRhs, AssignLhs, Context))) | ||
return true; | ||
|
||
if ((Op == BO_GT || Op == BO_GE) && | ||
(tidy::utils::areStatementsIdentical(CondLhs, AssignLhs, Context) && | ||
tidy::utils::areStatementsIdentical(CondRhs, AssignRhs, Context))) | ||
return true; | ||
|
||
return false; | ||
} | ||
|
||
static bool maxCondition(const BinaryOperator::Opcode Op, const Expr *CondLhs, | ||
const Expr *CondRhs, const Expr *AssignLhs, | ||
const Expr *AssignRhs, const ASTContext &Context) { | ||
if ((Op == BO_LT || Op == BO_LE) && | ||
(tidy::utils::areStatementsIdentical(CondLhs, AssignLhs, Context) && | ||
tidy::utils::areStatementsIdentical(CondRhs, AssignRhs, Context))) | ||
return true; | ||
|
||
if ((Op == BO_GT || Op == BO_GE) && | ||
(tidy::utils::areStatementsIdentical(CondLhs, AssignRhs, Context) && | ||
tidy::utils::areStatementsIdentical(CondRhs, AssignLhs, Context))) | ||
return true; | ||
|
||
return false; | ||
} | ||
|
||
QualType getNonTemplateAlias(QualType QT) { | ||
while (true) { | ||
// cast to a TypedefType | ||
if (const TypedefType *TT = dyn_cast<TypedefType>(QT)) { | ||
// check if the typedef is a template and if it is dependent | ||
if (!TT->getDecl()->getDescribedTemplate() && | ||
!TT->getDecl()->getDeclContext()->isDependentContext()) | ||
return QT; | ||
QT = TT->getDecl()->getUnderlyingType(); | ||
} | ||
// cast to elaborated type | ||
else if (const ElaboratedType *ET = dyn_cast<ElaboratedType>(QT)) { | ||
QT = ET->getNamedType(); | ||
} else { | ||
break; | ||
} | ||
} | ||
return QT; | ||
} | ||
|
||
static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs, | ||
const Expr *AssignLhs, | ||
const SourceManager &Source, | ||
const LangOptions &LO, | ||
StringRef FunctionName, | ||
const BinaryOperator *BO) { | ||
const llvm::StringRef CondLhsStr = Lexer::getSourceText( | ||
Source.getExpansionRange(CondLhs->getSourceRange()), Source, LO); | ||
const llvm::StringRef CondRhsStr = Lexer::getSourceText( | ||
Source.getExpansionRange(CondRhs->getSourceRange()), Source, LO); | ||
const llvm::StringRef AssignLhsStr = Lexer::getSourceText( | ||
Source.getExpansionRange(AssignLhs->getSourceRange()), Source, LO); | ||
|
||
clang::QualType GlobalImplicitCastType; | ||
clang::QualType LhsType = CondLhs->getType() | ||
.getCanonicalType() | ||
.getNonReferenceType() | ||
.getUnqualifiedType(); | ||
clang::QualType RhsType = CondRhs->getType() | ||
.getCanonicalType() | ||
.getNonReferenceType() | ||
.getUnqualifiedType(); | ||
if (LhsType != RhsType) { | ||
GlobalImplicitCastType = getNonTemplateAlias(BO->getLHS()->getType()); | ||
} | ||
|
||
return (AssignLhsStr + " = " + FunctionName + | ||
(!GlobalImplicitCastType.isNull() | ||
? "<" + GlobalImplicitCastType.getAsString() + ">(" | ||
: "(") + | ||
CondLhsStr + ", " + CondRhsStr + ");") | ||
.str(); | ||
} | ||
|
||
UseStdMinMaxCheck::UseStdMinMaxCheck(StringRef Name, ClangTidyContext *Context) | ||
: ClangTidyCheck(Name, Context), | ||
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle", | ||
utils::IncludeSorter::IS_LLVM), | ||
areDiagsSelfContained()) {} | ||
|
||
void UseStdMinMaxCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { | ||
Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle()); | ||
} | ||
|
||
void UseStdMinMaxCheck::registerMatchers(MatchFinder *Finder) { | ||
auto AssignOperator = | ||
binaryOperator(hasOperatorName("="), | ||
hasLHS(expr(unless(isTypeDependent())).bind("AssignLhs")), | ||
hasRHS(expr(unless(isTypeDependent())).bind("AssignRhs"))); | ||
auto BinaryOperator = | ||
binaryOperator(hasAnyOperatorName("<", ">", "<=", ">="), | ||
hasLHS(expr(unless(isTypeDependent())).bind("CondLhs")), | ||
hasRHS(expr(unless(isTypeDependent())).bind("CondRhs"))) | ||
.bind("binaryOp"); | ||
Finder->addMatcher( | ||
ifStmt(stmt().bind("if"), unless(isIfInMacro()), | ||
unless(hasElse(stmt())), // Ensure `if` has no `else` | ||
hasCondition(BinaryOperator), | ||
hasThen( | ||
anyOf(stmt(AssignOperator), | ||
compoundStmt(statementCountIs(1), has(AssignOperator)))), | ||
hasParent(stmt(unless(ifStmt(hasElse( | ||
equalsBoundNode("if"))))))), // Ensure `if` has no `else if` | ||
this); | ||
} | ||
|
||
void UseStdMinMaxCheck::registerPPCallbacks(const SourceManager &SM, | ||
Preprocessor *PP, | ||
Preprocessor *ModuleExpanderPP) { | ||
IncludeInserter.registerPreprocessor(PP); | ||
} | ||
|
||
void UseStdMinMaxCheck::check(const MatchFinder::MatchResult &Result) { | ||
const auto *If = Result.Nodes.getNodeAs<IfStmt>("if"); | ||
const clang::LangOptions &LO = Result.Context->getLangOpts(); | ||
const auto *CondLhs = Result.Nodes.getNodeAs<Expr>("CondLhs"); | ||
const auto *CondRhs = Result.Nodes.getNodeAs<Expr>("CondRhs"); | ||
const auto *AssignLhs = Result.Nodes.getNodeAs<Expr>("AssignLhs"); | ||
const auto *AssignRhs = Result.Nodes.getNodeAs<Expr>("AssignRhs"); | ||
const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("binaryOp"); | ||
const clang::BinaryOperatorKind BinaryOpcode = BinaryOp->getOpcode(); | ||
const SourceLocation IfLocation = If->getIfLoc(); | ||
const SourceLocation ThenLocation = If->getEndLoc(); | ||
|
||
auto ReplaceAndDiagnose = [&](const llvm::StringRef FunctionName) { | ||
const SourceManager &Source = *Result.SourceManager; | ||
diag(IfLocation, "use `%0` instead of `%1`") | ||
<< FunctionName << BinaryOp->getOpcodeStr() | ||
<< FixItHint::CreateReplacement( | ||
SourceRange(IfLocation, Lexer::getLocForEndOfToken( | ||
ThenLocation, 0, Source, LO)), | ||
createReplacement(CondLhs, CondRhs, AssignLhs, Source, LO, | ||
FunctionName, BinaryOp)) | ||
<< IncludeInserter.createIncludeInsertion( | ||
Source.getFileID(If->getBeginLoc()), AlgorithmHeader); | ||
}; | ||
|
||
if (minCondition(BinaryOpcode, CondLhs, CondRhs, AssignLhs, AssignRhs, | ||
(*Result.Context))) { | ||
ReplaceAndDiagnose("std::min"); | ||
} else if (maxCondition(BinaryOpcode, CondLhs, CondRhs, AssignLhs, AssignRhs, | ||
(*Result.Context))) { | ||
ReplaceAndDiagnose("std::max"); | ||
} | ||
} | ||
|
||
} // namespace clang::tidy::readability |
42 changes: 42 additions & 0 deletions
42
clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
//===--- UseStdMinMaxCheck.h - clang-tidy -----------------------*- C++ -*-===// | ||
// | ||
// 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 | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESTDMINMAXCHECK_H | ||
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESTDMINMAXCHECK_H | ||
|
||
#include "../ClangTidyCheck.h" | ||
#include "../utils/IncludeInserter.h" | ||
|
||
namespace clang::tidy::readability { | ||
|
||
/// Replaces certain conditional statements with equivalent calls to | ||
/// ``std::min`` or ``std::max``. | ||
/// For the user-facing documentation see: | ||
/// http://clang.llvm.org/extra/clang-tidy/checks/readability/UseStdMinMax.html | ||
class UseStdMinMaxCheck : public ClangTidyCheck { | ||
public: | ||
UseStdMinMaxCheck(StringRef Name, ClangTidyContext *Context); | ||
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { | ||
return LangOpts.CPlusPlus; | ||
} | ||
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, | ||
Preprocessor *ModuleExpanderPP) override; | ||
void storeOptions(ClangTidyOptions::OptionMap &Opts) override; | ||
void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
void check(const ast_matchers::MatchFinder::MatchResult &Result) override; | ||
std::optional<TraversalKind> getCheckTraversalKind() const override { | ||
return TK_IgnoreUnlessSpelledInSource; | ||
} | ||
|
||
private: | ||
utils::IncludeInserter IncludeInserter; | ||
}; | ||
|
||
} // namespace clang::tidy::readability | ||
|
||
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESTDMINMAXCHECK_H |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
29 changes: 29 additions & 0 deletions
29
clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
.. title:: clang-tidy - readability-use-std-min-max | ||
|
||
readability-use-std-min-max | ||
=========================== | ||
|
||
Replaces certain conditional statements with equivalent calls to | ||
``std::min`` or ``std::max``. | ||
Note: This may impact performance in critical code due to potential | ||
additional stores compared to the original if statement. | ||
|
||
Before: | ||
|
||
.. code-block:: c++ | ||
|
||
void foo() { | ||
int a = 2, b = 3; | ||
if (a < b) | ||
a = b; | ||
} | ||
|
||
|
||
After: | ||
|
||
.. code-block:: c++ | ||
|
||
void foo() { | ||
int a = 2, b = 3; | ||
a = std::max(a, b); | ||
} |
Oops, something went wrong.