[clang-tidy] Adding Fuchsia checker for overloaded operators

[juliehockett]

Summary

Adds a check to the Fuchsia module to warn if an operator is overloaded, except move and copy operators.

See https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md for reference.

[JonasToth]

What happens if the operator is overloaded outside a class? Is that allowed/disallowed and could you please mention the guideline on that in the docs + tests.

clang-tidy/fuchsia/OverloadedOperatorCheck.cpp

19

I think isOverloadedOperator is a better name.

clang-tidy/fuchsia/OverloadedOperatorCheck.h

20

I think this comment could be rephrased to a better sentence. Maybe Overloading operators is disallowed by the fuchsia coding standard.?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst

18

Could you make the link clickable?

[Eugene.Zelenko]

docs/ReleaseNotes.rst

141

assignment operators?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst

7

assignment operators?

[aaron.ballman]

clang-tidy/fuchsia/OverloadedOperatorCheck.cpp

19

I think isFuchsiaOverloadedOperator() is even better because of the extra work done by the checker (otherwise, it might be tempting to put this into ASTMatchers.h).

31

I think this could be better stated as "cannot overload %0" and pass in D (which should expand to something reasonable).

test/clang-tidy/fuchsia-overloaded-operator.cpp

12

While this is an overloaded assignment operator, it's grating for it not to accept a const B&.

[juliehockett]

Fixing comments

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst

18

Doesn't rST parse standalone links this into hyperlinks without additional markup?

[juliehockett]

Updating matcher to include overloaded operators outside classes.

[JonasToth]

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst

18

I am not sure but probably. It's ok then.

[aaron.ballman]

Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system headers?

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst

16

& should bind to other here.

test/clang-tidy/fuchsia-overloaded-operator.cpp

12

The & should bind to other. Same below. Also, the param identifier should start with a capital letter (also same below).

[juliehockett]

Fixing comments

[juliehockett]

Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system headers?

It would make sense for it to be run on the system headers when clang-tidy's -system-headers flag is included, otherwise not -- does that logic need to go into the checker though?

[aaron.ballman]

In D41363#962727, @juliehockett wrote:

Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system headers?

It would make sense for it to be run on the system headers when clang-tidy's -system-headers flag is included, otherwise not -- does that logic need to go into the checker though?

Ah, I forgot about that detail, so no, I don't think you need to make modifications here for that.

One more tiny nit with the test case, otherwise LGTM.

test/clang-tidy/fuchsia-overloaded-operator.cpp

18

Missed these.

clang-tidy/fuchsia/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyFuchsiaModule
   DefaultArgumentsCheck.cpp
   FuchsiaTidyModule.cpp
+  OverloadedOperatorCheck.cpp
   VirtualInheritanceCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

 //===--- FuchsiaTidyModule.cpp - clang-tidy--------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefaultArgumentsCheck.h"
+#include "OverloadedOperatorCheck.h"
 #include "VirtualInheritanceCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace fuchsia {
 
 /// This module is for Fuchsia-specific checks.
 class FuchsiaModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "fuchsia-default-arguments");
+    CheckFactories.registerCheck<OverloadedOperatorCheck>(
+        "fuchsia-overloaded-operator");
     CheckFactories.registerCheck<VirtualInheritanceCheck>(
         "fuchsia-virtual-inheritance");
   }
 };
 // Register the FuchsiaTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<FuchsiaModule>
     X("fuchsia-module", "Adds Fuchsia platform checks.");
 } // namespace fuchsia
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the FuchsiaModule.
 volatile int FuchsiaModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/fuchsia/OverloadedOperatorCheck.h

+//===--- OverloadedOperatorCheck.h - clang-tidy------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_OVERLOADED_OPERATOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_OVERLOADED_OPERATOR_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+/// Overloading operators is disallowed by the Fuchsia coding standard.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-overloaded-operator.html
+class OverloadedOperatorCheck : public ClangTidyCheck {
+public:
+  OverloadedOperatorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_OVERLOADED_OPERATOR_H

clang-tidy/fuchsia/OverloadedOperatorCheck.cpp

+//===--- OverloadedOperatorCheck.cpp - clang-tidy--------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "OverloadedOperatorCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+AST_MATCHER(CXXMethodDecl, isFuchsiaOverloadedOperator) {
+  if (Node.isCopyAssignmentOperator() || Node.isMoveAssignmentOperator())
+    return false;
+  return Node.isOverloadedOperator();
+}
+
+void OverloadedOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(cxxMethodDecl(isFuchsiaOverloadedOperator()).bind("decl"), this);
+}
+
+void OverloadedOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<CXXMethodDecl>("decl"))
+    diag(D->getLocStart(), "cannot overload %0") << D;
+}
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New `cppcoreguidelines-owning-memory <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-owning-memory.html>`_ check
 
   This check implements the type-based semantic of ``gsl::owner<T*>``, but without
   flow analysis.
 
 - New `fuchsia-default-arguments
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-default-arguments.html>`_ check
 
   Warns if a function or method is declared or called with default arguments.
-  
+- New `fuchsia-overloaded-operator
+  <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-overloaded-operator.html>`_ check
+
+  Warns if an operator is overloaded, except for the assignment (copy and move) operators.
+
 - New `fuchsia-virtual-inheritance
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-virtual-inheritance.html>`_ check
 
   Warns if classes are defined with virtual inheritance.
     
 - New `google-objc-avoid-throwing-exception
   <http://clang.llvm.org/extra/clang-tidy/checks/google-objc-avoid-throwing-exception.html>`_ check
 

docs/clang-tidy/checks/fuchsia-overloaded-operator.rst

+.. title:: clang-tidy - fuchsia-overloaded-operator
+
+fuchsia-overloaded-operator
+===========================
+
+Warns if an operator is overloaded, except for the assignment (copy and move) 
+operators.
+
+For example:
+
+.. code-block:: c++
+
+  int operator+(int);     // Warning
+
+  B &operator=(const B& other);  // No warning
+  B &operator=(B &&other) // No warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
    fuchsia-default-arguments
+   fuchsia-overloaded-operator
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-objc-avoid-throwing-exception

test/clang-tidy/fuchsia-overloaded-operator.cpp

+// RUN: %check_clang_tidy %s fuchsia-overloaded-operator %t
+
+class A {
+public:
+  int operator+(int);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: cannot overload 'operator+' [fuchsia-overloaded-operator]
+};
+
+class B {
+public:
+  B &operator=(const B& other);
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:3: warning: cannot overload 'operator=' [fuchsia-overloaded-operator]
+  B &operator=(B &&other);
+  // CHECK-MESSAGES-NOT: [[@LINE-1]]:3: warning: cannot overload 'operator=' [fuchsia-overloaded-operator]
+};