[clang-tidy] Add cppcoreguidelines-comparison-operator

[nullptr.cpp]

Summary

Finds comparison operators(==, !=, <, <=, >, >=) which are not
noexcept, are defined as member functions, have parameters of different type
or pass parameters not by value for cheap to copy types and not by reference to
const for expensive to copy types.

Examples:

struct A {
  int Var;
  bool operator==(const A &a) const; // 'operator==' should not be member function
};

struct B;
bool operator==(const B &lh, const B &rh); // 'operator==' should be marked noexcept

struct C;
bool operator==(const C &lh, int rh) noexcept; // 'operator==' should have 2 parameters of the same type

struct C { // cheap to copy
int Var;
};

bool operator==(const C &lh, const C &rh) noexcept;
// 'lh' of type 'C' is cheap to copy, should be passed by value
// 'rh' of type 'C' is cheap to copy, should be passed by value

struct D { // expensive to copy
int Array[1024];
};

bool operator==(D lh, D rh) noexcept;
// 'lh' of type 'D' is expensive to copy, should be passed by reference to const
// 'rh' of type 'D' is expensive to copy, should be passed by reference to const

Options:

• CheckParamPassing
  
  Boolean flag to warn when operators pass parameters not by value for cheap to copy types and not by reference to const for expensive to copy types. Default value is true.

• ExpensiveToCopySize
  
  When parameters have a size greater than ExpensiveToCopySize bytes, treat them as expensive to copy types. Default value is 0, which means to use 2 * sizeof(void*) as the threshold.

The relevant rules in the C++ Core Guidelines are:

• C.86: Make == symmetric with respect to operand types and noexcept
• C.161: Use non-member functions for symmetric operators
• F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp

27

Please use const auto * - type is spelled in same statement.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-comparison-operator.rst

10

Usually such text is placed at the end of documentation. See other checks.

[njames93]

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp

20–21

Is there a precedent to match spaceship operator?

48–52

We should likely only warn on the canonical declaration

struct A{
  bool operator==(const A&) const; // Warn here
};

bool A::operator==(const A&) const { // Don't warn here
  return true;
}

54–55

This code should not be executed when running in pre c++11 mode, as noexcept was added in c++11.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-comparison-operator.cpp

1

Once you rebase to trunk this test will fail without --fix-notes specified.

[nullptr.cpp]

Modify according to suggestions

[nullptr.cpp]

Update

[njames93]

While we're here, I'm not sure if there is a cppcoreguideline for it, but parameters to comparison functions should be passed by const ref usually, or by value if they are cheap to copy. Maybe there's precedent to extend this to flag parameters that take non const reference, R value reference or by value if not cheap to copy(There's bound to be a function somewhere in clang tidy that determines if a type is a cheap copy)

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp

48

It might just be better to return here if the matched result isn't the same as the canonical declaration.

54–55

I was saying the other checks can run in c++98 mode but then wrap this in an if block for c++11

[nullptr.cpp]

In D98692#2631182, @njames93 wrote:

While we're here, I'm not sure if there is a cppcoreguideline for it, but parameters to comparison functions should be passed by const ref usually, or by value if they are cheap to copy. Maybe there's precedent to extend this to flag parameters that take non const reference, R value reference or by value if not cheap to copy(There's bound to be a function somewhere in clang tidy that determines if a type is a cheap copy)

The relevant rule is F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const.
This should be done in another checker dedicated for parameter passing.

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp

48

Why only warn on the canonical declaration?
Clients need to find the definition and other declarations by themselves. I think it's better to warn about everything found here.

[njames93]

In D98692#2631504, @nullptr.cpp wrote:

The relevant rule is F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const.
This should be done in another checker dedicated for parameter passing.

That's not exactly a relevant rule, That describes handling of In parameters. However, the point I'm making is we should enforce that parameters to comparison functions are In parameters.

[nullptr.cpp]

• Warn on both declarations and definitions
• Add check for parameter passing
• Add options CheckParamPassing and ExpensiveToCopySize

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidGotoCheck.cpp
   AvoidNonConstGlobalVariablesCheck.cpp
+  ComparisonOperatorCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   PreferMemberInitializerCheck.cpp

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.h

+//===--- ComparisonOperatorCheck.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_CPPCOREGUIDELINES_COMPARISONOPERATORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_COMPARISONOPERATORCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Finds comparison operators which are not noexcept, define as member
+/// functions or have parameters of different type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-comparison-operator.html
+class ComparisonOperatorCheck : public ClangTidyCheck {
+public:
+  ComparisonOperatorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void diagCanThrow(const FunctionDecl *FD);
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_COMPARISONOPERATORCHECK_H

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp

+//===--- ComparisonOperatorCheck.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 "ComparisonOperatorCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void ComparisonOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(hasAnyOverloadedOperatorName("==", "!=", "<",
+                                                               "<=", ">", ">="))
+                         .bind("operator"),
+                     this);
+}
+
+static bool canThrow(const FunctionDecl *FD) {
+  const FunctionProtoType *Proto = FD->getType()->getAs<FunctionProtoType>();
+  if (isUnresolvedExceptionSpec(Proto->getExceptionSpecType()))
+    return false;
+
+  return Proto->canThrow() == CT_Can;
+}
+
+static bool hasSameParam(const FunctionDecl *FD) {
+  constexpr unsigned int ExpectedNumParams = 2;
+  if (FD->getNumParams() != ExpectedNumParams)
+    return false;
+
+  QualType LH = FD->getParamDecl(0)->getOriginalType();
+  QualType RH = FD->getParamDecl(1)->getOriginalType();
+
+  return LH == RH;
+}
+
+void ComparisonOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Operator = Result.Nodes.getNodeAs<FunctionDecl>("operator");
+
+  if (isa<CXXMethodDecl>(Operator)) {
+    diag(Operator->getLocation(), "%0 should not be member function")
+        << Operator;
+    return;
+  }
+
+  if (canThrow(Operator))
+    diagCanThrow(Operator);
+
+  if (!hasSameParam(Operator))
+    diag(Operator->getLocation(),
+         "%0 should have 2 parameters of the same type")
+        << Operator;
+}
+
+static SourceLocation getInsertionLoc(const FunctionDecl *FD) {
+  FunctionTypeLoc FTL = FD->getFunctionTypeLoc();
+  if (!FTL)
+    return SourceLocation();
+
+  SourceLocation RParenLoc = FTL.getRParenLoc();
+  return RParenLoc.getLocWithOffset(1);
+}
+
+void ComparisonOperatorCheck::diagCanThrow(const FunctionDecl *FD) {
+  diag(FD->getLocation(), "%0 should be marked noexcept") << FD;
+
+  DiagnosticBuilder FixDiag =
+      diag(FD->getLocation(), "mark 'noexcept'", DiagnosticIDs::Note);
+  SourceRange ExceptionSpecRange = FD->getExceptionSpecSourceRange();
+  SourceLocation InsertionLoc = getInsertionLoc(FD);
+
+  if (ExceptionSpecRange.isValid())
+    FixDiag << FixItHint::CreateReplacement(ExceptionSpecRange, "noexcept");
+  else if (InsertionLoc.isValid())
+    FixDiag << FixItHint::CreateInsertion(InsertionLoc, " noexcept");
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
+#include "ComparisonOperatorCheck.h"
 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "PreferMemberInitializerCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
+    CheckFactories.registerCheck<ComparisonOperatorCheck>(
+        "cppcoreguidelines-comparison-operator");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");
     CheckFactories.registerCheck<InitVariablesCheck>(
         "cppcoreguidelines-init-variables");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^
 
 - New :doc:`concurrency-thread-canceltype-asynchronous
   <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` check.
 
   Finds ``pthread_setcanceltype`` function calls where a thread's cancellation
   type is set to asynchronous.
 
+- New :doc:`cppcoreguidelines-comparison-operator
+  <clang-tidy/checks/cppcoreguidelines-comparison-operator>` check.
+
+  Finds comparison operators(``==``, ``!=``, ``<``, ``<=``, ``>``, ``>=``) which
+  are not ``noexcept``, define as member functions or have parameters of
+  different type.
+
 - New :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
 
   Finds member initializations in the constructor body which can be placed into
   the initialization list instead.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-comparison-operator.rst

+.. title:: clang-tidy - cppcoreguidelines-comparison-operator
+
+cppcoreguidelines-comparison-operator
+=====================================
+
+Finds comparison operators(``==``, ``!=``, ``<``, ``<=``, ``>``, ``>=``) which
+are not ``noexcept``, define as member functions or have parameters of different
+type.
+
+The relevant rule in the C++ Core Guidelines is
+`C.86: Make == symmetric with respect to operand types and noexcept
+<https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c86-make--symmetric-with-respect-to-operand-types-and-noexcept>`_.
+
+Examples:
+
+.. code-block:: c++
+
+  struct A {
+    int Var;
+    bool operator==(const A &a) const; // 'operator==' should not be member function
+  };
+
+.. code-block:: c++
+
+  struct C;
+  bool operator==(const C &lh, const C &rh); // 'operator==' should be marked noexcept
+
+.. code-block:: c++
+
+  struct B;
+  bool operator==(const B &lh, int rh) noexcept; // 'operator==' should have 2 parameters of the same type

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak <clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak.html>`_,
    `clang-analyzer-valist.CopyToSelf <clang-analyzer-valist.CopyToSelf.html>`_,
    `clang-analyzer-valist.Uninitialized <clang-analyzer-valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer-valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency-mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency-thread-canceltype-asynchronous.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_,
+   `cppcoreguidelines-comparison-operator <cppcoreguidelines-comparison-operator.html>`_, "Yes"
    `cppcoreguidelines-init-variables <cppcoreguidelines-init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines-interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines-macro-usage.html>`_,
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines-narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines-no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines-owning-memory.html>`_,
    `cppcoreguidelines-prefer-member-initializer <cppcoreguidelines-prefer-member-initializer.html>`_, "Yes"
    `cppcoreguidelines-pro-bounds-array-to-pointer-decay <cppcoreguidelines-pro-bounds-array-to-pointer-decay.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-comparison-operator.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-comparison-operator %t
+
+// member function
+namespace test_member_function {
+struct A {
+  int Var;
+
+  bool operator==(const A &a) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'operator==' should not be member function [cppcoreguidelines-comparison-operator]
+
+  bool operator!=(const A &a) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'operator!=' should not be member function [cppcoreguidelines-comparison-operator]
+
+  bool operator<(const A &a) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'operator<' should not be member function [cppcoreguidelines-comparison-operator]
+
+  bool operator<=(const A &a) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'operator<=' should not be member function [cppcoreguidelines-comparison-operator]
+
+  bool operator>(const A &a) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'operator>' should not be member function [cppcoreguidelines-comparison-operator]
+
+  bool operator>=(const A &a) const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'operator>=' should not be member function [cppcoreguidelines-comparison-operator]
+};
+} // namespace test_member_function
+
+// parameters types differ
+namespace test_params_type_differ {
+struct B;
+bool operator==(const B &lh, int rh) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator==' should have 2 parameters of the same type [cppcoreguidelines-comparison-operator]
+
+bool operator!=(const B &lh, int rh) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator!=' should have 2 parameters of the same type [cppcoreguidelines-comparison-operator]
+
+bool operator<(const B &lh, int rh) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator<' should have 2 parameters of the same type [cppcoreguidelines-comparison-operator]
+
+bool operator<=(const B &lh, int rh) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator<=' should have 2 parameters of the same type [cppcoreguidelines-comparison-operator]
+
+bool operator>(const B &lh, int rh) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator>' should have 2 parameters of the same type [cppcoreguidelines-comparison-operator]
+
+bool operator>=(const B &lh, int rh) noexcept;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator>=' should have 2 parameters of the same type [cppcoreguidelines-comparison-operator]
+} // namespace test_params_type_differ
+
+// can throw
+namespace test_can_throw {
+struct C;
+bool operator==(const C &lh, const C &rh);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator==' should be marked noexcept [cppcoreguidelines-comparison-operator]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: mark 'noexcept'
+// CHECK-FIXES: bool operator==(const C &lh, const C &rh) noexcept
+
+bool operator!=(const C &lh, const C &rh);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator!=' should be marked noexcept [cppcoreguidelines-comparison-operator]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: mark 'noexcept'
+// CHECK-FIXES: bool operator!=(const C &lh, const C &rh) noexcept;
+
+bool operator<(const C &lh, const C &rh);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator<' should be marked noexcept [cppcoreguidelines-comparison-operator]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: mark 'noexcept'
+// CHECK-FIXES: bool operator<(const C &lh, const C &rh) noexcept;
+
+bool operator<=(const C &lh, const C &rh);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator<=' should be marked noexcept [cppcoreguidelines-comparison-operator]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: mark 'noexcept'
+// CHECK-FIXES: bool operator<=(const C &lh, const C &rh) noexcept;
+
+bool operator>(const C &lh, const C &rh);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator>' should be marked noexcept [cppcoreguidelines-comparison-operator]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: mark 'noexcept'
+// CHECK-FIXES: bool operator>(const C &lh, const C &rh) noexcept;
+
+bool operator>=(const C &lh, const C &rh);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'operator>=' should be marked noexcept [cppcoreguidelines-comparison-operator]
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: mark 'noexcept'
+// CHECK-FIXES: bool operator>=(const C &lh, const C &rh) noexcept;
+} // namespace test_can_throw
+
+// ok
+namespace test_ok {
+struct D;
+bool operator==(const D &lh, const D &rh) noexcept;
+bool operator!=(const D &lh, const D &rh) noexcept;
+bool operator<(const D &lh, const D &rh) noexcept;
+bool operator<=(const D &lh, const D &rh) noexcept;
+bool operator>(const D &lh, const D &rh) noexcept;
+bool operator>=(const D &lh, const D &rh) noexcept;
+} // namespace test_ok