[clang-tidy] Add readability-constant-operand-order check #167158
+336
−8
Conversation
This file contains hidden or 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
…lace RUN flags correctly
This introduces a new clang-tidy readability check that enforces a consistent position for constant operands in binary comparison expressions. The check warns when a constant appears on the non-preferred side of an operator and can automatically fix simple cases (no side effects, not in macros). Configurable options: * PreferredConstantSide (Right | Left, default = Right) * BinaryOperators (comma-separated list, default = ==,!=,<,<=,>,>=) Example: if (0 == x) → if (x == 0) Includes tests, documentation, and integration with the Readability module.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
Member
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Sai Deepak Sana (saideepaksana) ChangesThis patch introduces a new clang-tidy check in the readability module that enforces consistent placement of constant operands in binary comparison expressions. Motivation: Behavior:
Configurable Options:
Examples: Files added/modified:
Testing: Reviewer suggestions: @EugeneZelenko @PiotrZSL @carlosgalvezp Patch is 20.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/167158.diff 8 Files Affected:
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<llvm::StringRef, 8> 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<IntegerLiteral>(E) || isa<FloatingLiteral>(E) ||
+ isa<StringLiteral>(E) || isa<CharacterLiteral>(E) ||
+ isa<CXXBoolLiteralExpr>(E) || isa<CXXNullPtrLiteralExpr>(E) ||
+ isa<GNUNullExpr>(E))
+ return true;
+
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+ if (isa<EnumConstantDecl>(DRE->getDecl()))
+ return true;
+ if (const auto *VD = dyn_cast<VarDecl>(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<BinaryOperator>("binop");
+ const auto *L = Result.Nodes.getNodeAs<Expr>("lhs");
+ const auto *R = Result.Nodes.getNodeAs<Expr>("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 <string>
+#include <vector>
+
+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<std::string> 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<ConstReturnTypeCheck>(
"readability-const-return-type");
+ CheckFactories.registerCheck<ConstantOperandOrderCheck>(
+ "readability-constant-operand-order");
CheckFactories.registerCheck<ContainerContainsCheck>(
"readability-container-contains");
CheckFactories.registerCheck<ContainerDataPointerCheck>(
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
+ <clang-tidy/checks/readability/constant-operand-order>` check.
+
+ FIXME: Write a short description.
+
- New :doc:`readability-redundant-parentheses
<clang-tidy/checks/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 <bugprone/unused-return-value>`,
:doc:`bugprone-use-after-move <bugprone/use-after-move>`,
:doc:`bugprone-virtual-near-miss <bugprone/virtual-near-miss>`, "Yes"
+ :doc:`cert-dcl58-cpp <cert/dcl58-cpp>`,
+ :doc:`cert-env33-c <cert/env33-c>`,
:doc:`cert-err33-c <cert/err33-c>`,
+ :doc:`cert-err52-cpp <cert/err52-cpp>`,
:doc:`cert-err60-cpp <cert/err60-cpp>`,
+ :doc:`cert-flp30-c <cert/flp30-c>`,
+ :doc:`cert-mem57-cpp <cert/mem57-cpp>`,
:doc:`cert-msc50-cpp <cert/msc50-cpp>`,
:doc:`cert-msc51-cpp <cert/msc51-cpp>`,
:doc:`cert-oop58-cpp <cert/oop58-cpp>`,
@@ -373,8 +378,9 @@ Clang-Tidy Checks
:doc:`readability-avoid-nested-conditional-operator <readability/avoid-nested-conditional-operator>`,
:doc:`readability-avoid-return-with-void-value <readability/avoid-return-with-void-value>`, "Yes"
:doc:`readability-avoid-unconditional-preprocessor-if <readability/avoid-unconditional-preprocessor-if>`,
- :doc:`readability-braces-around-statements <readability/braces-around-statements>`, "Yes"
+ :doc:`readability-braces-around-statements <readability/braces-around-statements>`,
:doc:`readability-const-return-type <readability/const-return-type>`, "Yes"
+ :doc:`readability-constant-operand-order <readability/constant-operand-order>`, "Yes"
:doc:`readability-container-contains <readability/container-contains>`, "Yes"
:doc:`readability-container-data-pointer <readability/container-data-pointer>`, "Yes"
:doc:`readability-container-size-empty <readability/container-size-empty>`, "Yes"
@@ -442,20 +448,15 @@ Check aliases
:doc:`cert-dcl50-cpp <cert/dcl50-cpp>`, :doc:`modernize-avoid-variadic-functions <modernize/avoid-variadic-functions>`,
:doc:`cert-dcl51-cpp <cert/dcl51-cpp>`, :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes"
:doc:`cert-dcl54-cpp <cert/dcl54-cpp>`, :doc:`misc-new-delete-overloads <misc/new-delete-overloads>`,
- :doc:`cert-dcl58-cpp <cert/dcl58-cpp>`, :doc:`bugprone-std-namespace-modification <bugprone/std-namespace-modification>`,
:doc:`cert-dcl59-cpp <cert/dcl59-cpp>`, :doc:`google-build-namespaces <google/build-namespaces>`,
- :doc:`cert-env33-c <cert/env33-c>`, :doc:`bugprone-command-processor <bugprone/command-processor>`,
:doc:`cert-err09-cpp <cert/err09-cpp>`, :doc:`misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference>`,
:doc:`cert-err34-c <cert/err34-c>`, :doc:`bugprone-unchecked-string-to-number-conversion <bugprone/unchecked-string-to-number-conversion>`,
- :doc:`cert-err52-cpp <cert/err52-cpp>`, :doc:`modernize-avoid-setjmp-longjmp <modernize/avoid-setjmp-longjmp>`,
:doc:`cert-err58-cpp <cert/err58-cpp>`, :doc:`bugprone-throwing-static-initialization <bugprone/throwing-static-initialization>`,
:doc:`cert-err61-cpp <cert/err61-cpp>`, :doc:`misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference>`,
:doc:`cert-exp42-c <cert/exp42-c>`, :doc:`bugprone-suspicious-memory-comparison <bugprone/suspicious-memory-comparison>`,
:doc:`cert-fio38-c <cert/fio38-c>`, :doc:`misc-non-copyable-objects <misc/non-copyable-objects>`,
- :doc:`cert-flp30-c <cert/flp30-c>`, :doc:`bugprone-float-loop-counter <bugprone/float-loop-counter>`,
:doc:`cert-flp37-c <cert/flp37-c>`, :doc:`bugprone-suspicious-memory-comparison <bugprone/suspicious-memory-comparison>`,
:doc:`cert-int09-c <cert/int09-c>`, :doc:`readability-enum-initial-value <readability/enum-initial-value>`, "Yes"
- :doc:`cert-mem57-cpp <cert/mem57-cpp>`, :doc:`bugprone-default-operator-new-on-overaligned-type <bugprone/default-operator-new-on-overaligned-type>`,
:doc:`cert-msc24-c <cert/msc24-c>`, :doc:`bugprone-unsafe-functions <bugprone/unsafe-functions>`,
:doc:`cert-msc30-c <cert/msc30-c>`, :doc:`cert-msc50-cpp <cert/msc50-cpp>`,
:doc:`cert-msc32-c <cert/msc32-c>`, :doc:`cert-msc51-cpp <cert/msc51-cpp>`,
@@ -578,12 +579,12 @@ Check aliases
:doc:`cppcoreguidelines-non-private-member-variables-in-classes <cppcoreguidelines/non-private-member-variables-in-classes>`, :doc:`misc-non-private-member-variables-in-classes <misc/non-private-member-variables-in-classes>`,
:doc:`cppcoreguidelines-use-default-member-init <cppcoreguidelines/use-default-member-init>`, :doc:`modernize-use-default-member-init <modernize/use-default-member-init>`, "Yes"
:doc:`fuchsia-header-anon-namespaces <fuchsia/header-anon-namespaces>`, :doc:`google-build-namespaces <google/build-namespaces>`,
- :doc:`google-readability-braces-around-statements <google/readability-braces-around-statements>`, :doc:`readability-braces-around-statements <readability/braces-around-statements>`, "Yes"
+ :doc:`google-readability-braces-around-statements <google/readability-braces-around-statements>`, :doc:`readability-braces-around-statements <readability/braces-around-statements>`,
:doc:`google-readability-function-size <google/readability-function-size>`, :doc:`readability-function-size <readability/function-size>`,
:doc:`google-readability-namespace-comments <google/readability-namespace-comments>`, :doc:`llvm-namespace-comment <llvm/namespace-comment>`,
:doc:`hicpp-avoid-c-arrays <hicpp/avoid-c-arrays>`, :doc:`modernize-avoid-c-arrays <modernize/avoid-c-arrays>`,
:doc:`hicpp-avoid-goto <hicpp/avoid-goto>`, :doc:`cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto>`,
- :doc:`hicpp-braces-around-statements <hicpp/braces-around-statements>`, :doc:`readability-braces-around-statements <readability/braces-around-statements>`, "Yes"
+ :doc:`hicpp-braces-around-statements <hicpp/braces-around-statements>`, :doc:`readability-braces-around-statements <readability/braces-around-statements>`,
:doc:`hicpp-deprecated-headers <hicpp/deprecated-headers>`, :doc:`modernize-deprecated-headers <modernize/deprecated-headers>`, "Yes"
:doc:`hicpp-explicit-conversions <hicpp/explicit-conversions>`, :doc:`google-explicit-constructor <google/explicit-constructor>`, "Yes"
:doc:`hicpp-function-size <hicpp/function-size>`, :doc:`readability-function-size <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...
[truncated]
|
EugeneZelenko
requested review from
HerrCai0907,
carlosgalvezp and
localspook
|
|
||
| // ------------------------ helpers ------------------------ | ||
|
|
||
| namespace { |
Contributor
There was a problem hiding this comment.
No need for anonymous namespace. static is enough.
Comment on lines
+172
to
+173
| llvm::StringRef OpName = Bin->getOpcodeStr(); | ||
| std::string NewOp = invertOperatorText(OpName); |
Contributor
There was a problem hiding this comment.
Suggested change
| llvm::StringRef OpName = Bin->getOpcodeStr(); | |
| std::string NewOp = invertOperatorText(OpName); | |
| const StringRef OpName = Bin->getOpcodeStr(); | |
| const std::string NewOp = invertOperatorText(OpName); |
Same below.
Comment on lines
+52
to
+53
|
|
||
| // Implementation helpers live in the .cpp file. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Implementation helpers live in the .cpp file. |
Useless comment.
| - New :doc:`readability-constant-operand-order | ||
| <clang-tidy/checks/readability/constant-operand-order>` check. | ||
|
|
||
| FIXME: Write a short description. |
Contributor
There was a problem hiding this comment.
Missing one sentence description.
| :doc:`bugprone-unused-return-value <bugprone/unused-return-value>`, | ||
| :doc:`bugprone-use-after-move <bugprone/use-after-move>`, | ||
| :doc:`bugprone-virtual-near-miss <bugprone/virtual-near-miss>`, "Yes" | ||
| :doc:`cert-dcl58-cpp <cert/dcl58-cpp>`, |
clang-tools-extra/docs/clang-tidy/checks/readability/constant-operand-order.rst
Show resolved
Hide resolved
|
|
||
| .. option:: PreferredConstantSide (string) | ||
|
|
||
| Either ``Left`` or ``Right``. Default: ``Right``. |
Contributor
There was a problem hiding this comment.
Suggested change
| Either ``Left`` or ``Right``. Default: ``Right``. | |
| Either `Left` or `Right`. Default: `Right`. |
Lowercase?
|
|
||
| .. option:: BinaryOperators (string) | ||
|
|
||
| Comma-separated list of operators to check. Default: ``==,!=,<,<=,>,>=``. |
Contributor
There was a problem hiding this comment.
Suggested change
| Comma-separated list of operators to check. Default: ``==,!=,<,<=,>,>=``. | |
| Comma-separated list of operators to check. Default: `==,!=,<,<=,>,>=`. |
You can test this locally with the following command:git diff -U0 origin/main...HEAD -- clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.cpp clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.h clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp |
python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \
-path build -p1 -quietView the output from clang-tidy here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch introduces a new clang-tidy check in the readability module that enforces consistent placement of constant operands in binary comparison expressions.
Motivation:
Many codebases prefer a consistent order such as
if (x == 0)instead ofif (0 == x). The rule improves readability and avoids “Yoda conditions.”Historically, some codebases preferred constants on the left to prevent accidental assignment (
=instead of==). This check supports both styles.Behavior:
The check detects when exactly one operand of a binary comparison is a constant and verifies whether it’s on the preferred side.
Configurable Options:
PreferredConstantSide(default ="Right", also supports"Left")"BinaryOperators(default = "==,!=,<,<=,>,>=")Examples:
Files added/modified:
clang-tidy/readability/ConstantOperandOrderCheck.cppclang-tidy/readability/ConstantOperandOrderCheck.htest/clang-tidy/checkers/readability/constant-operand-order.cppdocs/clang-tidy/checks/readability-constant-operand-order.rstdocs/clang-tidy/checks/list.rstReadabilityTidyModule.cppTesting:
The checker passes its
llvm-littest suite locally and supports both left/right configuration modes.Reviewer suggestions: @EugeneZelenko @PiotrZSL @carlosgalvezp