[clang-tidy] Add readability-redundant-using check

[nullptr.cpp]

Summary

Finds redundant using declarations and directives.

Example:

namespace n {
void func();
}

namespace n {
using n::func; // redundant using declaration, already in namespace 'n'.
}

namespace n {
using namespace n; // redundant using directive, already in namespace 'n'.
}

[Eugene.Zelenko]

I think this check belongs to readability module.

clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp

1 ↗

(On Diff #325995)

Please make single line from two. See other checks as example.

[njames93]

clang-tools-extra/clang-tidy/misc/RedundantUsingCheck.cpp

23 ↗

(On Diff #325995)

Why restrict these to main file only?

30–33 ↗

(On Diff #325995)

nit: Elide braces of single line if statements.

35–38 ↗

(On Diff #325995)

45 ↗

(On Diff #325995)

nit: don't use auto as type isn't explicitly spelled in the initialization.

50 ↗

(On Diff #325995)

Shouldn't there be a break after we have diagnosed this?

78 ↗

(On Diff #325995)

nit: The canonical way to do diagnostics like this is with select. Then pass 0 for declaration and 1 for directive.

[nullptr.cpp]

Modify according to suggestions

[balazske]

clang-tools-extra/test/clang-tidy/checkers/readability-redundant-using.cpp

114 ↗

(On Diff #326320)

Add more tests?

namespace outer {
namespace inner {
void func();
} // namespace inner
using inner::func();
} // namespace outer

using namespace outer;
using namespace inner;

[njames93]

Can you override the isLanguageVersionSupported method to ensure this check only runs over c++ code.
Can you add some macro tests, typically we err on the side of caution and don't emit any fixes for macro code. You can use SourceLocation::IsMacroID() to detect macro expansions.
The description looks to have an error in the 2nd example, guess you are messing 'n' after the namespace.
How will this handle inline namespaces.

namespace N {
inline namespace M {
void foo();
void bar();
} // namespace M
inline namespace O {
void bar();
}
using M::foo; // This is redundant.
using M::bar; // This is gonna give you a hard time.
} // namespace N

clang-tools-extra/clang-tidy/readability/RedundantUsingCheck.cpp

62 ↗

(On Diff #326320)

nit: New line between methods.

[balazske]

You can look into misc/UnusedUsingDeclsCheck.cpp (if not done yet), that check handles using too. And that check is related to this check, the module should be the same too (misc)?

[balazske]

There should be already a name lookup implemented somewhere in clang (DeclContext? or Sema) that maybe usable here (check if the name of the "used" entity is visible in the declaration context of the using statement). Probably it is not as simple to do because the using is already there and it may depend on the order.

[Eugene.Zelenko]

In D97361#2589673, @balazske wrote:

You can look into misc/UnusedUsingDeclsCheck.cpp (if not done yet), that check handles using too. And that check is related to this check, the module should be the same too (misc)?

Majority of redundant checks are in readability. Awhile ago I suggested separate group for unused checks or move them to Clang.

[nullptr.cpp]

• Restrict to only run on C++ code
• Ignore using defined in macro
• Support inline namespace
• Support global namespace

[nullptr.cpp]

Ping

[nullptr.cpp]

Update

[nullptr.cpp]

Rebase

[nullptr.cpp]

In D97361#2609616, @nullptr.cpp wrote:

• Restrict to only run on C++ code
• Ignore using defined in macro
• Support inline namespace
• Support global namespace

Ping.

[nullptr.cpp]

Ping.

[nullptr.cpp]

Ping

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

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.CPlusPlus11;
+  }
+  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 auto *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");
+  const FunctionDecl *CanonOperator = Operator->getCanonicalDecl();
+
+  if (isa<CXXMethodDecl>(CanonOperator)) {
+    diag(CanonOperator->getLocation(), "%0 should not be member function")
+        << CanonOperator;
+    return;
+  }
+
+  if (canThrow(CanonOperator))
+    diagCanThrow(CanonOperator);
+
+  if (!hasSameParam(CanonOperator))
+    diag(CanonOperator->getLocation(),
+         "%0 should have 2 parameters of the same type")
+        << CanonOperator;
+}
+
+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.
+
+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
+
+
+The relevant rules in the C++ Core Guidelines are:
+
+- `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>`_
+
+- `C.161: Use non-member functions for symmetric operators
+  <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c161-use-non-member-functions-for-symmetric-operators>`_

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 -- --fix-notes
+
+// 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
+
+// only warn on the canonical declaration
+namespace test_canonical_declaration {
+// 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 A::operator==(const A &) const { // Don't warn here
+  return true;
+}
+
+// parameters types 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 { // Don't warn here
+  return true;
+}
+
+// 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) { // Don't warn here
+  return true;
+}
+} // namespace test_canonical_declaration