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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:29
+
+bool isPotentialSymmetricBinaryOperator(OverloadedOperatorKind Kind) {
+  return llvm::is_contained(
----------------
again, I would probably try to generate simple switch with those macros., and this function should be static.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:48
+
+bool isComparisonOperator(OverloadedOperatorKind Kind) {
+  std::bitset<NUM_OVERLOADED_OPERATORS> BitSetCompOps{};
----------------
For me this function is overkill, I would use simple switch and let compiler optimize it. except that should be static.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:62-68
+AST_MATCHER(CXXMethodDecl, isPotentialSymmetricBinaryOperator) {
+  return isPotentialSymmetricBinaryOperator(Node.getOverloadedOperator());
+}
+
+AST_MATCHER(CXXMethodDecl, isComparisonOperator) {
+  return isComparisonOperator(Node.getOverloadedOperator());
+}
----------------
put matchers into anonymous namespace....


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:164
+    StringRef Name, ClangTidyContext *Context)
+    : TransformerClangTidyCheck(
+          makeRewriteRule(Context->getLangOpts().CPlusPlus20), Name, Context) {}
----------------
Consider writing this check as an normal simple one without using TransformerClangTidyCheck as base. After that half of this code won't be needed, and it will be very easy to maintain/fix/extend it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861