[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:24-34
+using transformer::applyFirst;
+using transformer::cat;
+using transformer::changeTo;
+using transformer::insertBefore;
+using transformer::makeRule;
+using transformer::name;
+using transformer::node;
----------------
Just bring the whole namespace into scope.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:56
+bool isComparisonOperator(OverloadedOperatorKind Kind) {
+  return llvm::is_contained(
+      std::initializer_list<OverloadedOperatorKind>{
----------------
Could these not be represented as a bitset?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:84
+
+static Expected<DynTypedNode> getNode(const BoundNodes &Nodes, StringRef ID) {
+  auto &NodesMap = Nodes.getMap();
----------------
Should there ever be a case where this returns an error. If not can this assert instead and just return a DynTypedNode?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:117-122
+      for (; OptToken.hasValue() && !IsConstToken(OptToken.getValue()) &&
+             !OptToken.getValue().isOneOf(tok::TokenKind::l_brace,
+                                          tok::TokenKind::kw_default,
+                                          tok::TokenKind::semi);
+           OptToken = GetNextToken(OptToken.getValue().getLocation())) {
+      }
----------------
Looks like this is begging to be a while loop.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:129-133
+      // This error is unreachable within this check. constSpec is only called
+      // on defaulted symmetric binary operators, which are required to be const
+      // specified.
+      return invalidArgumentError(
+          "constSpec invalid argument: member function is not const qualified");
----------------
If its unreachable, use unreachable to indicate that.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:134
+          "constSpec invalid argument: member function is not const qualified");
+    };
+    return invalidArgumentError(
----------------
This semi-colon looks suspicious


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:135-136
+    };
+    return invalidArgumentError(
+        "constSpec invalid argument: not a CXXMethodDecl");
+  };
----------------
Is this error reachable? Again if not use unreachable.
If it is reachable, is it possible to show a test case where this error is expected and detected in the test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861