[clang-tidy] Add readability-constant-operand-order check

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

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).
+  const StringRef OpName = Bin->getOpcodeStr();
+  const 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

clang-tools-extra/clang-tidy/readability/ConstantOperandOrderCheck.h

@@ -0,0 +1,56 @@
+//===----------------------------------------------------------------------===//
+//
+// 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. "=="
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTANTOPERANDORDERCHECK_H

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>(

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.
 

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>`,

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 note 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: `==,!=,<,<=,>,>=`.