[clang-tidy] Remove obsolete checker google-runtime-references

[baloghadamsoftware]

Summary

The rules which is the base of this checker is removed from the Google C++ Style Guide in May: Update C++ styleguide. Now this checker became obsolete.

[Eugene.Zelenko]

Does this check make sense in content of other style guides?

[lebedev.ri]

In D88831#2311800, @Eugene.Zelenko wrote:

Does this check make sense in content of other style guides?

I'd +1 to moving it to readability.

[baloghadamsoftware]

In D88831#2311802, @lebedev.ri wrote:

In D88831#2311800, @Eugene.Zelenko wrote:

Does this check make sense in content of other style guides?

I'd +1 to moving it to readability.

IMHO this check never made any sense. There was a Google-specific rule for this, that is why it was placed in google not in readability. Generally, forcing programmers not to use non-const reference parameters and force them the old C way (pointers) does not make any sense. I wonder why this rule was there in Google, but outside of it I never saw such a rule. I would surely not place such check into readability because following such rule reduces the readability instead of improving it. However, the main point is that even for Google there is no such rule anymore thus I see no point to keep this check.

[gribozavr2]

Google C++ style guide does not have this rule anymore. Thanks for the cleanup!

[aaron.ballman]

LGTM to remove rather than move into readability unless someone has a good argument as to why it belongs there. I don't think this pattern makes code more readable in general as it recommends using a more verbose syntax (pointers, which require dereferencing) over a safer, concise syntax (references, which carry semantic information about nullness).

[aaron.ballman]

One request though: can you add a release note about the removal with a brief explanation of why it was removed?

[lebedev.ri]

@hans it would be nice if this could make it into the release,
removing stuff is usually a pretty safe thing to do :)

[hans]

In D88831#2316453, @lebedev.ri wrote:

@hans it would be nice if this could make it into the release,
removing stuff is usually a pretty safe thing to do :)

I think it's too late for this release, sorry. I also don't see any particular reason to rush this one.

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

add_clang_library(clangTidyGoogleModule
   DefaultArgumentsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   FunctionNamingCheck.cpp
   GlobalNamesInHeadersCheck.cpp
   GlobalVariableDeclarationCheck.cpp
   GoogleTidyModule.cpp
   IntegerTypesCheck.cpp
-  NonConstReferences.cpp
   OverloadedUnaryAndCheck.cpp
   TodoCommentCheck.cpp
   UnnamedNamespaceInHeaderCheck.cpp
   UpgradeGoogletestCaseCheck.cpp
   UsingNamespaceDirectiveCheck.cpp
 
   LINK_LIBS
   clangTidy

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

 #include "AvoidUnderscoreInGoogletestNameCheck.h"
 #include "DefaultArgumentsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
 #include "FunctionNamingCheck.h"
 #include "GlobalNamesInHeadersCheck.h"
 #include "GlobalVariableDeclarationCheck.h"
 #include "IntegerTypesCheck.h"
-#include "NonConstReferences.h"
 #include "OverloadedUnaryAndCheck.h"
 #include "TodoCommentCheck.h"
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UpgradeGoogletestCaseCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 
 using namespace clang::ast_matchers;
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<objc::FunctionNamingCheck>(
         "google-objc-function-naming");
     CheckFactories.registerCheck<objc::GlobalVariableDeclarationCheck>(
         "google-objc-global-variable-declaration");
     CheckFactories.registerCheck<runtime::IntegerTypesCheck>(
         "google-runtime-int");
     CheckFactories.registerCheck<runtime::OverloadedUnaryAndCheck>(
         "google-runtime-operator");
-    CheckFactories.registerCheck<runtime::NonConstReferences>(
-        "google-runtime-references");
     CheckFactories
         .registerCheck<readability::AvoidUnderscoreInGoogletestNameCheck>(
             "google-readability-avoid-underscore-in-googletest-name");
     CheckFactories.registerCheck<readability::AvoidCStyleCastsCheck>(
         "google-readability-casting");
     CheckFactories.registerCheck<readability::TodoCommentCheck>(
         "google-readability-todo");
     CheckFactories

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

-//===--- NonConstReferences.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_GOOGLE_NON_CONST_REFERENCES_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NON_CONST_REFERENCES_H
-
-#include "../ClangTidyCheck.h"
-
-namespace clang {
-namespace tidy {
-namespace google {
-namespace runtime {
-
-/// 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);
-  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
-    return LangOpts.CPlusPlus;
-  }
-  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
-  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
-
-private:
-  const std::vector<std::string> IncludedTypes;
-};
-
-} // namespace runtime
-} // namespace google
-} // namespace tidy
-} // namespace clang
-
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_NON_CONST_REFERENCES_H

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

-//===--- NonConstReferences.cpp - 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
-//
-//===----------------------------------------------------------------------===//
-
-#include "NonConstReferences.h"
-#include "../utils/OptionsUtils.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 {
-
-NonConstReferences::NonConstReferences(StringRef Name,
-                                       ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      IncludedTypes(
-          utils::options::parseStringList(Options.get("IncludedTypes", ""))) {}
-
-void NonConstReferences::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "IncludedTypes",
-                utils::options::serializeStringList(IncludedTypes));
-}
-
-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->getLocation().isMacroID())
-    return;
-
-  if (!Function->isCanonicalDecl())
-    return;
-
-  if (const auto *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");
-
-  if (std::find_if(IncludedTypes.begin(), IncludedTypes.end(),
-                   [&](llvm::StringRef ExplicitType) {
-                     return ReferencedType.getCanonicalType().getAsString(
-                                Result.Context->getPrintingPolicy()) ==
-                            ExplicitType;
-                   }) != IncludedTypes.end())
-    return;
-
-  // 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;
-
-  if (Parameter->getName().empty()) {
-    diag(Parameter->getLocation(), "non-const reference parameter at index %0, "
-                                   "make it const or use a pointer")
-        << Parameter->getFunctionScopeIndex();
-  } else {
-    diag(Parameter->getLocation(),
-         "non-const reference parameter %0, make it const or use a pointer")
-        << Parameter;
-  }
-}
-
-} // namespace runtime
-} // namespace google
-} // namespace tidy
-} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 
   Added an option `GetConfigPerFile` to support including files which use
   different naming styles.
 
+- Removed `google-runtime-references` check because the rule it checks does
+  not exist in the Google Style Guide anymore.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

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

-.. title:: clang-tidy - google-runtime-references
-
-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
-
-
-Options
--------
-
-.. option:: IncludedTypes
-
-   A semicolon-separated list of names of types to explicitly include. Default is empty.

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

-// RUN: %check_clang_tidy %s google-runtime-references %t -- \
-// RUN:   -config="{CheckOptions: \
-// RUN:             [{key: google-runtime-references.IncludedTypes, \
-// RUN:               value: 'included::A; included::B'}]}"
-
-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 at index 2, {{.*}}
-
-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 at index 1, {{.*}}
-
-// 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; }
-
-namespace included {
-class A {};
-class B {};
-void f7(A &);
-void f8(B &);
-}
-void f9(included::A &);
-void f10(included::B &);
-
-#define DEFINE_F(name) void name(int& a)
-
-DEFINE_F(func) {}