diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index 161a0d96caf41..d8237d3d5dda2 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyReadabilityModule STATIC AvoidUnconditionalPreprocessorIfCheck.cpp BracesAroundStatementsCheck.cpp ConstReturnTypeCheck.cpp + ConstantOperandOrderCheck.cpp ContainerContainsCheck.cpp ContainerDataPointerCheck.cpp ContainerSizeEmptyCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.cpp b/clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.cpp new file mode 100644 index 0000000000000..215c736141adb --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.cpp @@ -0,0 +1,197 @@ +//===----------------------------------------------------------------------===// +// +// 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 "ConstantOperandOrderCheck.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/OperationKinds.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +/// Out-of-line ctor so vtable is emitted. +ConstantOperandOrderCheck::ConstantOperandOrderCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) { + // Read options (StringRef -> std::string). + PreferredSide = Options.get(PreferredSideOption, "Right").str(); + + // Parse BinaryOperators option (comma-separated). + std::string OpsCSV = + Options.get(BinaryOperatorsOption, "==,!=,<,<=,>,>=").str(); + + llvm::SmallVector Tokens; + llvm::StringRef(OpsCSV).split(Tokens, ','); + Operators.clear(); + for (auto Tok : Tokens) { + llvm::StringRef Trim = Tok.trim(); + if (!Trim.empty()) + Operators.emplace_back(Trim.str()); + } +} + +ConstantOperandOrderCheck::~ConstantOperandOrderCheck() = default; + +void ConstantOperandOrderCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, PreferredSideOption, PreferredSide); + Options.store(Opts, BinaryOperatorsOption, + llvm::join(Operators.begin(), Operators.end(), ",")); +} + +// ------------------------ helpers ------------------------ + +namespace { + +static const Expr *strip(const Expr *E) { + return E ? E->IgnoreParenImpCasts() : nullptr; +} + +static bool isSimpleConstantExpr(const Expr *E) { + E = strip(E); + if (!E) + return false; + + if (isa(E) || isa(E) || + isa(E) || isa(E) || + isa(E) || isa(E) || + isa(E)) + return true; + + if (const auto *DRE = dyn_cast(E)) { + if (isa(DRE->getDecl())) + return true; + if (const auto *VD = dyn_cast(DRE->getDecl())) + return VD->isConstexpr() || VD->getType().isConstQualified(); + } + + return false; +} + +static bool hasSideEffectsExpr(const Expr *E, ASTContext &Ctx) { + E = strip(E); + return E && E->HasSideEffects(Ctx); +} + +static std::string invertOperatorText(llvm::StringRef Op) { + if (Op == "<") + return ">"; + if (Op == ">") + return "<"; + if (Op == "<=") + return ">="; + if (Op == ">=") + return "<="; + // symmetric: ==, != + return Op.str(); +} + +} // namespace + +// ------------------------ matchers ------------------------ + +void ConstantOperandOrderCheck::registerMatchers(MatchFinder *Finder) { + if (Operators.empty()) + return; + + for (const auto &Op : Operators) { + Finder->addMatcher(binaryOperator(hasOperatorName(Op), + hasLHS(expr().bind("lhs")), + hasRHS(expr().bind("rhs"))) + .bind("binop"), + this); + } +} + +// ------------------------ check / fixit ------------------------ + +void ConstantOperandOrderCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Bin = Result.Nodes.getNodeAs("binop"); + const auto *L = Result.Nodes.getNodeAs("lhs"); + const auto *R = Result.Nodes.getNodeAs("rhs"); + if (!Bin || !L || !R) + return; + + const ASTContext &Ctx = *Result.Context; + SourceManager &SM = *Result.SourceManager; + + const Expr *LCore = strip(L); + const Expr *RCore = strip(R); + const bool LIsConst = isSimpleConstantExpr(LCore); + const bool RIsConst = isSimpleConstantExpr(RCore); + + // Only when exactly one side is constant. + if (LIsConst == RIsConst) + return; + + const bool PreferRight = (PreferredSide == "Right"); + + // If it's already on the preferred side -> nothing to do. + if ((PreferRight && RIsConst && !LIsConst) || + (!PreferRight && LIsConst && !RIsConst)) + return; + + // At this point: exactly one side is constant, and it's on the *wrong* side. + // Emit diagnosis (tests expect a warning even when we won't provide a fix). + auto D = + diag(Bin->getOperatorLoc(), "constant operand should be on the %0 side") + << PreferredSide; + + // Conservative: don't offer fix-its if swapping would move side-effects or if + // we're inside a macro expansion. + const bool LSE = L->HasSideEffects(Ctx); + const bool RSE = R->HasSideEffects(Ctx); + const bool AnyMacro = L->getBeginLoc().isMacroID() || + R->getBeginLoc().isMacroID() || + Bin->getOperatorLoc().isMacroID(); + if (LSE || RSE || AnyMacro) + return; // warning-only: no FixIt attached. + + // Get token ranges for the two operands. + CharSourceRange LRange = CharSourceRange::getTokenRange(L->getSourceRange()); + CharSourceRange RRange = CharSourceRange::getTokenRange(R->getSourceRange()); + if (LRange.isInvalid() || RRange.isInvalid()) + return; + + llvm::StringRef LText = Lexer::getSourceText(LRange, SM, Ctx.getLangOpts()); + llvm::StringRef RText = Lexer::getSourceText(RRange, SM, Ctx.getLangOpts()); + if (LText.empty() || RText.empty()) + return; + + // Compute operator replacement (invert for asymmetric operators). + llvm::StringRef OpName = Bin->getOpcodeStr(); + std::string NewOp = invertOperatorText(OpName); + + // Apply operand swaps as two independent replacements (safer than replacing + // the whole Bin range). + // Replace left operand with right text: + D << FixItHint::CreateReplacement(LRange, RText.str()); + // Replace right operand with left text: + D << FixItHint::CreateReplacement(RRange, LText.str()); + + // If needed, replace the operator token too. + if (NewOp != OpName.str()) { + // Compute an operator token range robustly: operator start and end. + SourceLocation OpStart = Bin->getOperatorLoc(); + SourceLocation OpEnd = + Lexer::getLocForEndOfToken(OpStart, 0, SM, Ctx.getLangOpts()); + if (OpStart.isValid() && OpEnd.isValid()) { + SourceRange OpRange(OpStart, OpEnd); + CharSourceRange OpTok = CharSourceRange::getTokenRange(OpRange); + if (!OpTok.isInvalid()) + D << FixItHint::CreateReplacement(OpTok, NewOp); + } + } +} + +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.h b/clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.h new file mode 100644 index 0000000000000..c776fade2a1e3 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.h @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// 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_CONSTANTOPERANDORDERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTANTOPERANDORDERCHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +#include +#include + +namespace clang::tidy::readability { +/// Finds binary expressions where the constant operand appears on the +/// non-preferred side (configurable) and offers a fix-it that swaps operands +/// (and inverts the operator for asymmetric operators like `<` / `>`). +/// Options +/// - BinaryOperators: comma-separated list of operators to check +/// (default: "==,!=,<,<=,>,>=") +/// - PreferredConstantSide: "Left" or "Right" (default: "Right") +class ConstantOperandOrderCheck : public ClangTidyCheck { +public: + ConstantOperandOrderCheck(StringRef Name, ClangTidyContext *Context); + ~ConstantOperandOrderCheck() override; + + // Persist options so they show up in config files. + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + // This check is primarily for C/C++, keep it limited to C++ for now. + return LangOpts.CPlusPlus; + } + +private: + // Option keys (used when storing & reading options) + static constexpr llvm::StringLiteral BinaryOperatorsOption = + "BinaryOperators"; + static constexpr llvm::StringLiteral PreferredSideOption = + "PreferredConstantSide"; + + // Runtime values, populated from Options in the constructor (or storeOptions) + std::string PreferredSide; // "Left" or "Right" + std::vector Operators; // list of operator names, e.g. "==" + + // Implementation helpers live in the .cpp file. +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTANTOPERANDORDERCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index afb63571de583..e87233b6c9aea 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -16,6 +16,7 @@ #include "AvoidUnconditionalPreprocessorIfCheck.h" #include "BracesAroundStatementsCheck.h" #include "ConstReturnTypeCheck.h" +#include "ConstantOperandOrderCheck.h" #include "ContainerContainsCheck.h" #include "ContainerDataPointerCheck.h" #include "ContainerSizeEmptyCheck.h" @@ -86,6 +87,8 @@ class ReadabilityModule : public ClangTidyModule { "readability-braces-around-statements"); CheckFactories.registerCheck( "readability-const-return-type"); + CheckFactories.registerCheck( + "readability-constant-operand-order"); CheckFactories.registerCheck( "readability-container-contains"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 666865cfb2fcd..7a56733614023 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -226,6 +226,11 @@ New checks Finds virtual function overrides with different visibility than the function in the base class. +- New :doc:`readability-constant-operand-order + ` check. + + FIXME: Write a short description. + - New :doc:`readability-redundant-parentheses ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index e2875604af72b..bc115571de1f0 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -178,8 +178,13 @@ Clang-Tidy Checks :doc:`bugprone-unused-return-value `, :doc:`bugprone-use-after-move `, :doc:`bugprone-virtual-near-miss `, "Yes" + :doc:`cert-dcl58-cpp `, + :doc:`cert-env33-c `, :doc:`cert-err33-c `, + :doc:`cert-err52-cpp `, :doc:`cert-err60-cpp `, + :doc:`cert-flp30-c `, + :doc:`cert-mem57-cpp `, :doc:`cert-msc50-cpp `, :doc:`cert-msc51-cpp `, :doc:`cert-oop58-cpp `, @@ -373,8 +378,9 @@ Clang-Tidy Checks :doc:`readability-avoid-nested-conditional-operator `, :doc:`readability-avoid-return-with-void-value `, "Yes" :doc:`readability-avoid-unconditional-preprocessor-if `, - :doc:`readability-braces-around-statements `, "Yes" + :doc:`readability-braces-around-statements `, :doc:`readability-const-return-type `, "Yes" + :doc:`readability-constant-operand-order `, "Yes" :doc:`readability-container-contains `, "Yes" :doc:`readability-container-data-pointer `, "Yes" :doc:`readability-container-size-empty `, "Yes" @@ -442,20 +448,15 @@ Check aliases :doc:`cert-dcl50-cpp `, :doc:`modernize-avoid-variadic-functions `, :doc:`cert-dcl51-cpp `, :doc:`bugprone-reserved-identifier `, "Yes" :doc:`cert-dcl54-cpp `, :doc:`misc-new-delete-overloads `, - :doc:`cert-dcl58-cpp `, :doc:`bugprone-std-namespace-modification `, :doc:`cert-dcl59-cpp `, :doc:`google-build-namespaces `, - :doc:`cert-env33-c `, :doc:`bugprone-command-processor `, :doc:`cert-err09-cpp `, :doc:`misc-throw-by-value-catch-by-reference `, :doc:`cert-err34-c `, :doc:`bugprone-unchecked-string-to-number-conversion `, - :doc:`cert-err52-cpp `, :doc:`modernize-avoid-setjmp-longjmp `, :doc:`cert-err58-cpp `, :doc:`bugprone-throwing-static-initialization `, :doc:`cert-err61-cpp `, :doc:`misc-throw-by-value-catch-by-reference `, :doc:`cert-exp42-c `, :doc:`bugprone-suspicious-memory-comparison `, :doc:`cert-fio38-c `, :doc:`misc-non-copyable-objects `, - :doc:`cert-flp30-c `, :doc:`bugprone-float-loop-counter `, :doc:`cert-flp37-c `, :doc:`bugprone-suspicious-memory-comparison `, :doc:`cert-int09-c `, :doc:`readability-enum-initial-value `, "Yes" - :doc:`cert-mem57-cpp `, :doc:`bugprone-default-operator-new-on-overaligned-type `, :doc:`cert-msc24-c `, :doc:`bugprone-unsafe-functions `, :doc:`cert-msc30-c `, :doc:`cert-msc50-cpp `, :doc:`cert-msc32-c `, :doc:`cert-msc51-cpp `, @@ -578,12 +579,12 @@ Check aliases :doc:`cppcoreguidelines-non-private-member-variables-in-classes `, :doc:`misc-non-private-member-variables-in-classes `, :doc:`cppcoreguidelines-use-default-member-init `, :doc:`modernize-use-default-member-init `, "Yes" :doc:`fuchsia-header-anon-namespaces `, :doc:`google-build-namespaces `, - :doc:`google-readability-braces-around-statements `, :doc:`readability-braces-around-statements `, "Yes" + :doc:`google-readability-braces-around-statements `, :doc:`readability-braces-around-statements `, :doc:`google-readability-function-size `, :doc:`readability-function-size `, :doc:`google-readability-namespace-comments `, :doc:`llvm-namespace-comment `, :doc:`hicpp-avoid-c-arrays `, :doc:`modernize-avoid-c-arrays `, :doc:`hicpp-avoid-goto `, :doc:`cppcoreguidelines-avoid-goto `, - :doc:`hicpp-braces-around-statements `, :doc:`readability-braces-around-statements `, "Yes" + :doc:`hicpp-braces-around-statements `, :doc:`readability-braces-around-statements `, :doc:`hicpp-deprecated-headers `, :doc:`modernize-deprecated-headers `, "Yes" :doc:`hicpp-explicit-conversions `, :doc:`google-explicit-constructor `, "Yes" :doc:`hicpp-function-size `, :doc:`readability-function-size `, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/constant-operand-order.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/constant-operand-order.rst new file mode 100644 index 0000000000000..592aa5d085f9f --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/constant-operand-order.rst @@ -0,0 +1,32 @@ +.. title:: clang-tidy - readability-constant-operand-order + +readability-constant-operand-order +================================== + +Warns when a constant appears on the non-preferred side of a supported binary +operator and offers a fix-it to swap operands (and invert the operator for +``<``, ``>``, ``<=``, ``>=``). + +Examples +-------- + +.. code-block:: c++ + + // Before + if (nullptr == p) { /* ... */ } + if (0 < x) { /* ... */ } + + // After + if (p == nullptr) { /* ... */ } + if (x > 0) { /* ... */ } + +Options +------- + +.. option:: PreferredConstantSide (string) + + Either ``Left`` or ``Right``. Default: ``Right``. + +.. option:: BinaryOperators (string) + + Comma-separated list of operators to check. Default: ``==,!=,<,<=,>,>=``. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/constant-operand-order.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/constant-operand-order.cpp new file mode 100644 index 0000000000000..c65c37b16e745 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/constant-operand-order.cpp @@ -0,0 +1,31 @@ +// RUN: %check_clang_tidy %s readability-constant-operand-order %t -- -- -std=c++17 +// RUN: %check_clang_tidy %s readability-constant-operand-order %t -- -check-suffixes=LEFT -- -config="{CheckOptions:[{key: readability-constant-operand-order.PreferredConstantSide, value: Left}, {key: readability-constant-operand-order.BinaryOperators, value: '==,!='}]}" -- -std=c++17 + +void swap_eq(int a) { + if (0 == a) {} // CHECK-MESSAGES: warning: constant operand should be on the Right side + // CHECK-FIXES: if (a == 0) {} // +} + +void swap_asym(int a) { + if (0 < a) {} // CHECK-MESSAGES: warning: + // CHECK-FIXES: if (a > 0) {} // +} + +void null_ptr(int *p) { + if (nullptr == p) {} // CHECK-MESSAGES: warning: + // CHECK-FIXES: if (p == nullptr) {} // +} + +// No-fix when side effects: +int g(); +void side_effects(int a) { + if (0 == g()) {} // CHECK-MESSAGES: warning: + // CHECK-FIXES-NOT: if (g() == 0) +} + +// Config variant: allow Left and only ==,!= +void left_ok(int a) { + if (0 == a) {} // CHECK-MESSAGES: warning: + // CHECK-FIXES: if (a == 0) {} // + // CHECK-LEFT-MESSAGES-NOT: readability-constant-operand-order +}