[clang-tidy] Add a checker that warns on const string & members.

[bkramer]

Summary

Those are considered unsafe and should be replaced with simple pointers or
full copies. It recognizes both std::string and ::string.

[bkramer]

Remove useless link, there doesn't seem to be a reachable version of this.

[alexfh]

Tests?

[bkramer]

Add test case that got attached to the wrong patch somehow.

[djasper]

clang-tidy/google/CMakeLists.txt

8

I'd probably call this StringReferenceMemberCheck, but I don't have a strong objection against this name.

clang-tidy/google/GoogleTidyModule.cpp

35

Where does "runtime" come from? Does that make sense?

clang-tidy/google/MemberStringReferencesCheck.h

22

Just for my benefit, why are non-const string reference members ok?

[bkramer]

clang-tidy/google/CMakeLists.txt

8

Less backwards that suggestion is, rename the file I will.

clang-tidy/google/GoogleTidyModule.cpp

35

We inherited the categories from cpplint.py and I've been following the existing style. I think it makes sense to have a direct mapping from cpplint categories to tidy checks.

clang-tidy/google/MemberStringReferencesCheck.h

22

The rationale is that you can initialize the following class with a const char *, creating a string temporary and leave a dangling reference. This behavior won't come up with non-const refs.

struct S {
  S(const string &S) : Str(S) {}

  const string &Str;
};

I'll add that info to the checker description comment.

[alexfh]

clang-tidy/google/GoogleTidyModule.cpp

35

Right. Having references to cpplint.py in some form is valueable, as we could then use the same check identifiers to support targeted NOLINT suppressions already present in the code.

The categories ("runtime", "readability", "build", "compatibility" etc.) used in cpplint.py make some sense: they usually reflect which aspects of the code quality the check addresses.

In this case "runtime" means that patterns detected by this check may lead to runtime errors (as opposed to just readability issues or compatibility problems).

clang-tidy/google/MemberStringReferencesCheck.cpp

28

Did you mean "in template instantiations"? We don't (and shouldn't) ignore members in template declarations, if the type is not dependent on template arguments.

[bkramer]

Rename check, add more doxygen comments.

[alexfh]

Looks good provided Daniel's concerns are addressed.

[djasper]

Sure.

[bkramer]

Closed by commit rL213133 (authored by d0k).

clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   GoogleTidyModule.cpp
+  MemberStringReferencesCheck.cpp
   NamedParameterCheck.cpp
   OverloadedUnaryAndCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.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 "AvoidCStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
+#include "MemberStringReferencesCheck.h"
 #include "NamedParameterCheck.h"
 #include "OverloadedUnaryAndCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 class GoogleModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "google-build-explicit-make-pair",
         new ClangTidyCheckFactory<build::ExplicitMakePairCheck>());
     CheckFactories.addCheckFactory(
         "google-explicit-constructor",
         new ClangTidyCheckFactory<ExplicitConstructorCheck>());
     CheckFactories.addCheckFactory(
+        "google-runtime-member-string-references",
+        new ClangTidyCheckFactory<runtime::MemberStringReferencesCheck>());
+    CheckFactories.addCheckFactory(
         "google-runtime-operator",
         new ClangTidyCheckFactory<runtime::OverloadedUnaryAndCheck>());
     CheckFactories.addCheckFactory(
         "google-readability-casting",
         new ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>());
     CheckFactories.addCheckFactory(
         "google-readability-function",
         new ClangTidyCheckFactory<readability::NamedParameterCheck>());

clang-tidy/google/MemberStringReferencesCheck.h

+//===--- MemberStringReferenceCheck.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_MEMBER_STRING_REF_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMBER_STRING_REF_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace runtime {
+
+/// \brief Finds members of type 'const string&'.
+///
+/// const string reference members are generally considered unsafe. This check
+/// emit warnings for both std::string and ::string const reference members.
+///
+/// Corresponding cpplint.py check name: 'runtime/member_string_reference'.
+class MemberStringReferencesCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace runtime
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMBER_STRING_REF_CHECK_H

clang-tidy/google/MemberStringReferencesCheck.cpp

+//===--- MemberStringReferenceCheck.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 "MemberStringReferencesCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace runtime {
+
+void MemberStringReferencesCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  // Look for const references to std::string or ::string.
+  auto String = anyOf(recordDecl(hasName("::std::basic_string")),
+                      recordDecl(hasName("::string")));
+  auto ConstString = qualType(isConstQualified(), hasDeclaration(String));
+
+  // Ignore members in templates.
+  auto InTemplate = hasAncestor(
+      decl(anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
+                 functionDecl(ast_matchers::isTemplateInstantiation()))));
+
+  Finder->addMatcher(fieldDecl(hasType(references(ConstString)),
+                               unless(InTemplate)).bind("member"),
+                     this);
+}
+
+void
+MemberStringReferencesCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Member = Result.Nodes.getNodeAs<FieldDecl>("member");
+  diag(Member->getLocStart(), "const string& members are dangerous. It is much "
+                              "better to use alternatives, such as pointers or "
+                              "simple constants.");
+}
+
+} // namespace runtime
+} // namespace tidy
+} // namespace clang

test/clang-tidy/google-member-string-references.cpp

+// RUN: clang-tidy %s -checks='-*,google-runtime-member-string-references' -- | FileCheck %s -implicit-check-not="{{warning|error}}:"
+
+namespace std {
+template<typename T>
+  class basic_string {};
+
+typedef basic_string<char> string;
+}
+
+class string {};
+
+
+struct A {
+  const std::string &s;
+// CHECK: :[[@LINE-1]]:3: warning: const string& members are dangerous. It is much better to use alternatives, such as pointers or simple constants.
+};
+
+struct B {
+  std::string &s;
+};
+
+struct C {
+  const std::string s;
+};
+
+template <typename T>
+struct D {
+  D();
+  const T &s;
+  const std::string &s2;
+// CHECK: :[[@LINE-1]]:3: warning: const string& members are dangerous. It is much better to use alternatives, such as pointers or simple constants.
+};
+
+D<std::string> d;
+
+struct AA {
+  const string &s;
+// CHECK: :[[@LINE-1]]:3: warning: const string& members are dangerous. It is much better to use alternatives, such as pointers or simple constants.
+};
+
+struct BB {
+  string &s;
+};
+
+struct CC {
+  const string s;
+};
+
+D<string> dd;