[clang-tidy] Add non-constant references in function parameters check.

[hokein]

Summary

This is implemented originally by Alexander Kornienko.

[hokein]

Fix code style.

[alexfh]

Thanks! A few comments.

clang-tidy/google/NonConstReferences.cpp

54 ↗

(On Diff #46510)

This could be auto to avoid duplicating the type name.

81 ↗

(On Diff #46510)

The return should be on the next line (clang-format -style=llvm should get this right).

105 ↗

(On Diff #46510)

We can avoid calling (the more expensive) getNameAsString here:

if (Function->getDeclName().isIdentifier() && Function->getName() == "swap") ...

109 ↗

(On Diff #46510)

I'd better avoid calling getAsString here as well, but I don't see an easy way to do this.

test/clang-tidy/google-runtime-references.cpp

1 ↗

(On Diff #46510)

I think, this test can use the %check_clang_tidy script, since it doesn't seem to do anything the script doesn't support (you'll need to s/CHECK/CHECK-MESSAGES/ to make it work, though).

[hokein]

Address comments.

[alexfh]

Looks good with one nit.

Thank you!

test/clang-tidy/google-runtime-references.cpp

1 ↗

(On Diff #46628)

-std=c++11 is added by default. Please remove everything after the %t.

[lebedev.ri]

// Don't warn on dependent types in templates.

Hmm, i wanted to try to fix my Bug 32683 about templated references being ignored, and i found out that it is actually intentional...
Reading https://google.github.io/styleguide/cppguide.html#Reference_Arguments i do not see why non-const T & should be ignored.
Is there a reason for this? Or perhaps, looking at the g9()/g10(), this implicit whitelisting should be more fine-grained, specific?

clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   GlobalNamesInHeadersCheck.cpp
   GoogleTidyModule.cpp
   IntegerTypesCheck.cpp
   MemsetZeroLengthCheck.cpp
+  NonConstReferences.cpp
   OverloadedUnaryAndCheck.cpp
   StringReferenceMemberCheck.cpp
   TodoCommentCheck.cpp
   UnnamedNamespaceInHeaderCheck.cpp
   UsingNamespaceDirectiveCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   )

clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp

 #include "../readability/RedundantSmartptrGetCheck.h"
 #include "AvoidCStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "IntegerTypesCheck.h"
 #include "MemsetZeroLengthCheck.h"
 #include "OverloadedUnaryAndCheck.h"
+#include "NonConstReferences.h"
 #include "StringReferenceMemberCheck.h"
 #include "TodoCommentCheck.h"
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<build::UsingNamespaceDirectiveCheck>(
         "google-build-using-namespace");
     CheckFactories.registerCheck<ExplicitConstructorCheck>(
         "google-explicit-constructor");
     CheckFactories.registerCheck<runtime::IntegerTypesCheck>(
         "google-runtime-int");
     CheckFactories.registerCheck<runtime::OverloadedUnaryAndCheck>(
         "google-runtime-operator");
+    CheckFactories.registerCheck<runtime::NonConstReferences>(
+        "google-runtime-references");
     CheckFactories.registerCheck<runtime::StringReferenceMemberCheck>(
         "google-runtime-member-string-references");
     CheckFactories.registerCheck<runtime::MemsetZeroLengthCheck>(
         "google-runtime-memset");
     CheckFactories.registerCheck<readability::AvoidCStyleCastsCheck>(
         "google-readability-casting");
     CheckFactories.registerCheck<readability::TodoCommentCheck>(
         "google-readability-todo");

clang-tools-extra/trunk/clang-tidy/google/NonConstReferences.h

+//===--- NonConstReferences.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_GOOGLE_NON_CONST_REFERENCES_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NON_CONST_REFERENCES_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace runtime {
+
+/// \brief Checks the usage of non-constant references in function parameters.
+///
+/// https://google.github.io/styleguide/cppguide.html#Reference_Arguments
+class NonConstReferences : public ClangTidyCheck {
+public:
+  NonConstReferences(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace runtime
+} // namespace google
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NON_CONST_REFERENCES_H

clang-tools-extra/trunk/clang-tidy/google/NonConstReferences.cpp

+//===--- NonConstReferences.cpp - clang-tidy --------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NonConstReferences.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace google {
+namespace runtime {
+
+void NonConstReferences::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      parmVarDecl(
+          unless(isInstantiated()),
+          hasType(references(
+              qualType(unless(isConstQualified())).bind("referenced_type"))),
+          unless(hasType(rValueReferenceType())))
+          .bind("param"),
+      this);
+}
+
+void NonConstReferences::check(const MatchFinder::MatchResult &Result) {
+  const auto *Parameter = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+  const auto *Function =
+      dyn_cast_or_null<FunctionDecl>(Parameter->getParentFunctionOrMethod());
+
+  if (Function == nullptr || Function->isImplicit())
+    return;
+
+  if (!Function->isCanonicalDecl())
+    return;
+
+  if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(Function)) {
+    // Don't warn on implementations of an interface using references.
+    if (Method->begin_overridden_methods() != Method->end_overridden_methods())
+      return;
+    // Don't warn on lambdas, as they frequently have to conform to the
+    // interface defined elsewhere.
+    if (Method->getParent()->isLambda())
+      return;
+  }
+
+  auto ReferencedType = *Result.Nodes.getNodeAs<QualType>("referenced_type");
+  // Don't warn on function references, they shouldn't be constant.
+  if (ReferencedType->isFunctionProtoType())
+    return;
+
+  // Don't warn on dependent types in templates.
+  if (ReferencedType->isDependentType())
+    return;
+
+  if (Function->isOverloadedOperator()) {
+    switch (Function->getOverloadedOperator()) {
+      case clang::OO_LessLess:
+      case clang::OO_PlusPlus:
+      case clang::OO_MinusMinus:
+      case clang::OO_PlusEqual:
+      case clang::OO_MinusEqual:
+      case clang::OO_StarEqual:
+      case clang::OO_SlashEqual:
+      case clang::OO_PercentEqual:
+      case clang::OO_LessLessEqual:
+      case clang::OO_GreaterGreaterEqual:
+      case clang::OO_PipeEqual:
+      case clang::OO_CaretEqual:
+      case clang::OO_AmpEqual:
+        // Don't warn on the first parameter of operator<<(Stream&, ...),
+        // operator++, operator-- and operation+assignment operators.
+        if (Function->getParamDecl(0) == Parameter)
+          return;
+        break;
+      case clang::OO_GreaterGreater: {
+        auto isNonConstRef = [](clang::QualType T) {
+          return T->isReferenceType() &&
+                 !T.getNonReferenceType().isConstQualified();
+        };
+        // Don't warn on parameters of stream extractors:
+        //   Stream& operator>>(Stream&, Value&);
+        // Both parameters should be non-const references by convention.
+        if (isNonConstRef(Function->getParamDecl(0)->getType()) &&
+            (Function->getNumParams() < 2 || // E.g. member operator>>.
+             isNonConstRef(Function->getParamDecl(1)->getType())) &&
+            isNonConstRef(Function->getReturnType()))
+          return;
+        break;
+      }
+      default:
+        break;
+    }
+  }
+
+  // Some functions use references to comply with established standards.
+  if (Function->getDeclName().isIdentifier() && Function->getName() == "swap")
+    return;
+
+  // iostream parameters are typically passed by non-const reference.
+  if (StringRef(ReferencedType.getAsString()).endswith("stream"))
+    return;
+
+  diag(Parameter->getLocation(),
+       "non-const reference parameter '%0', make it const or use a pointer")
+      << Parameter->getName();
+}
+
+} // namespace runtime
+} // namespace google
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/google-runtime-references.rst

+google-runtime-references
+=========================
+
+Checks the usage of non-constant references in function parameters.
+
+The corresponding style guide rule:
+https://google.github.io/styleguide/cppguide.html#Reference_Arguments

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    google-readability-function-size
    google-readability-namespace-comments
    google-readability-redundant-smartptr-get
    google-readability-todo
    google-runtime-int
    google-runtime-member-string-references
    google-runtime-memset
    google-runtime-operator
+   google-runtime-references
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion

clang-tools-extra/trunk/test/clang-tidy/google-runtime-references.cpp

+// RUN: %check_clang_tidy %s google-runtime-references %t -- -- -std=c++11
+
+int a;
+int &b = a;
+int *c;
+void f1(int a);
+void f2(int *b);
+void f3(const int &c);
+void f4(int const &d);
+
+// Don't warn on implicit operator= in c++11 mode.
+class A {
+  virtual void f() {}
+};
+// Don't warn on rvalue-references.
+struct A2 {
+  A2(A2&&) = default;
+  void f(A2&&) {}
+};
+
+// Don't warn on iostream parameters.
+namespace xxx {
+class istream { };
+class ostringstream { };
+}
+void g1(xxx::istream &istr);
+void g1(xxx::ostringstream &istr);
+
+void g1(int &a);
+// CHECK-MESSAGES: [[@LINE-1]]:14: warning: non-const reference parameter 'a', make it const or use a pointer [google-runtime-references]
+
+struct s {};
+void g2(int a, int b, s c, s &d);
+// CHECK-MESSAGES: [[@LINE-1]]:31: warning: non-const reference parameter 'd', {{.*}}
+
+typedef int &ref;
+void g3(ref a);
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: non-const reference {{.*}}
+
+void g4(int &a, int &b, int &);
+// CHECK-MESSAGES: [[@LINE-1]]:14: warning: non-const reference parameter 'a', {{.*}}
+// CHECK-MESSAGES: [[@LINE-2]]:22: warning: non-const reference parameter 'b', {{.*}}
+// CHECK-MESSAGES: [[@LINE-3]]:30: warning: non-const reference parameter '', {{.*}}
+
+class B {
+  B(B& a) {}
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: non-const reference {{.*}}
+  virtual void f(int &a) {}
+// CHECK-MESSAGES: [[@LINE-1]]:23: warning: non-const reference {{.*}}
+  void g(int &b);
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: non-const reference {{.*}}
+
+  // Don't warn on the parameter of stream extractors defined as members.
+  B& operator>>(int& val) { return *this; }
+};
+
+// Only warn on the first declaration of each function to reduce duplicate
+// warnings.
+void B::g(int &b) {}
+
+// Don't warn on the first parameter of stream inserters.
+A& operator<<(A& s, int&) { return s; }
+// CHECK-MESSAGES: [[@LINE-1]]:25: warning: non-const reference parameter '', {{.*}}
+
+// Don't warn on either parameter of stream extractors. Both need to be
+// non-const references by convention.
+A& operator>>(A& input, int& val) { return input; }
+
+// Don't warn on lambdas.
+auto lambda = [] (int&) {};
+
+// Don't warn on typedefs, as we'll warn on the function itself.
+typedef int (*fp)(int &);
+
+// Don't warn on function references.
+typedef void F();
+void g5(const F& func) {}
+void g6(F& func) {}
+
+template<typename T>
+void g7(const T& t) {}
+
+template<typename T>
+void g8(T t) {}
+
+void f5() {
+  g5(f5);
+  g6(f5);
+  g7(f5);
+  g7<F&>(f5);
+  g8(f5);
+  g8<F&>(f5);
+}
+
+// Don't warn on dependent types.
+template<typename T>
+void g9(T& t) {}
+template<typename T>
+void g10(T t) {}
+
+void f6() {
+  int i;
+  float f;
+  g9<int>(i);
+  g9<const int>(i);
+  g9<int&>(i);
+  g10<int&>(i);
+  g10<float&>(f);
+}
+
+// Warn only on the overridden methods from the base class, as the child class
+// only implements the interface.
+class C : public B {
+  C();
+  virtual void f(int &a) {}
+};
+
+// Don't warn on operator<< with streams-like interface.
+A& operator<<(A& s, int) { return s; }
+
+// Don't warn on swap().
+void swap(C& c1, C& c2) {}
+
+// Don't warn on standalone operator++, operator--, operator+=, operator-=,
+// operator*=, etc. that all need non-const references to be functional.
+A& operator++(A& a) { return a; }
+A operator++(A& a, int) { return a; }
+A& operator--(A& a) { return a; }
+A operator--(A& a, int) { return a; }
+A& operator+=(A& a, const A& b) { return a; }
+A& operator-=(A& a, const A& b) { return a; }
+A& operator*=(A& a, const A& b) { return a; }
+A& operator/=(A& a, const A& b) { return a; }
+A& operator%=(A& a, const A& b) { return a; }
+A& operator<<=(A& a, const A& b) { return a; }
+A& operator>>=(A& a, const A& b) { return a; }
+A& operator|=(A& a, const A& b) { return a; }
+A& operator^=(A& a, const A& b) { return a; }
+A& operator&=(A& a, const A& b) { return a; }