[PATCH] D98692: [clang-tidy] Add cppcoreguidelines-comparison-operator

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp:20-21
+void ComparisonOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(hasAnyOverloadedOperatorName("==", "!=", "<",
+                                                               "<=", ">", ">="))
+                         .bind("operator"),
----------------
Is there a precedent to match spaceship operator?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp:48-52
+  if (isa<CXXMethodDecl>(Operator)) {
+    diag(Operator->getLocation(), "%0 should not be member function")
+        << Operator;
+    return;
+  }
----------------
We should likely only warn on the canonical declaration
```lang=c++
struct A{
  bool operator==(const A&) const; // Warn here
};

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


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp:54-55
+
+  if (canThrow(Operator))
+    diagCanThrow(Operator);
+
----------------
This code should not be executed when running in pre c++11 mode, as noexcept was added in c++11.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-comparison-operator.cpp:1
+// RUN: %check_clang_tidy %s cppcoreguidelines-comparison-operator %t
+
----------------
Once you rebase to trunk this test will fail without `--fix-notes` specified.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98692/new/

https://reviews.llvm.org/D98692