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

[David Blaikie]

On Wed, Jul 16, 2014 at 3:00 AM, Benjamin Kramer
<benny.kra at googlemail.com> wrote:
> Author: d0k
> Date: Wed Jul 16 05:00:14 2014
> New Revision: 213133
>
> URL: http://llvm.org/viewvc/llvm-project?rev=213133&view=rev
> Log:
> [clang-tidy] Add a checker that warns on const string & members.

Why strings specifically? I guess this is what was checked for in cpplint.py?

At least the Google style guide mostly pushes on the actual ctor
parameter type - that that type should be a pointer (in the same way
Google style uses pointers to indicate mutability, it also espouses
them to indicate "is stored/used beyond the lifetime of this
function/ctor call"). This seems like a case where someone fixed an
issue too narrowly, and a step or two from the root cause, and we
maybe shouldn't duplicate that and instead diagnose the 'root' cause.

>
> Summary:
> Those are considered unsafe and should be replaced with simple pointers or
> full copies. It recognizes both std::string and ::string.
>
> Reviewers: alexfh, djasper
>
> Subscribers: cfe-commits
>
> Differential Revision: http://reviews.llvm.org/D4522
>
> Added:
>     clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.cpp
>     clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.h
>     clang-tools-extra/trunk/test/clang-tidy/google-member-string-references.cpp
> Modified:
>     clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
>     clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp
>
> Modified: clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt?rev=213133&r1=213132&r2=213133&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt (original)
> +++ clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt Wed Jul 16 05:00:14 2014
> @@ -7,6 +7,7 @@ add_clang_library(clangTidyGoogleModule
>    GoogleTidyModule.cpp
>    NamedParameterCheck.cpp
>    OverloadedUnaryAndCheck.cpp
> +  StringReferenceMemberCheck.cpp
>
>    LINK_LIBS
>    clangAST
>
> Modified: clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp?rev=213133&r1=213132&r2=213133&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp (original)
> +++ clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp Wed Jul 16 05:00:14 2014
> @@ -15,6 +15,7 @@
>  #include "ExplicitMakePairCheck.h"
>  #include "NamedParameterCheck.h"
>  #include "OverloadedUnaryAndCheck.h"
> +#include "StringReferenceMemberCheck.h"
>
>  using namespace clang::ast_matchers;
>
> @@ -34,6 +35,9 @@ public:
>          "google-runtime-operator",
>          new ClangTidyCheckFactory<runtime::OverloadedUnaryAndCheck>());
>      CheckFactories.addCheckFactory(
> +        "google-runtime-member-string-references",
> +        new ClangTidyCheckFactory<runtime::StringReferenceMemberCheck>());
> +    CheckFactories.addCheckFactory(
>          "google-readability-casting",
>          new ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>());
>      CheckFactories.addCheckFactory(
>
> Added: clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.cpp?rev=213133&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.cpp (added)
> +++ clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.cpp Wed Jul 16 05:00:14 2014
> @@ -0,0 +1,48 @@
> +//===--- StringReferenceMemberCheck.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 "StringReferenceMemberCheck.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 StringReferenceMemberCheck::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 template instantiations.
> +  auto InTemplateInstantiation = hasAncestor(
> +      decl(anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
> +                 functionDecl(ast_matchers::isTemplateInstantiation()))));
> +
> +  Finder->addMatcher(fieldDecl(hasType(references(ConstString)),
> +                               unless(InTemplateInstantiation)).bind("member"),
> +                     this);
> +}
> +
> +void
> +StringReferenceMemberCheck::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
>
> Added: clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.h
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.h?rev=213133&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.h (added)
> +++ clang-tools-extra/trunk/clang-tidy/google/StringReferenceMemberCheck.h Wed Jul 16 05:00:14 2014
> @@ -0,0 +1,50 @@
> +//===--- StringReferenceMemberCheck.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_STRING_REF_MEMBER_CHECK_H
> +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_STRING_REF_MEMBER_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 as they can
> +/// be created from a temporary quite easily.
> +///
> +/// \code
> +/// struct S {
> +///  S(const string &Str) : Str(Str) {}
> +///  const string &Str;
> +/// };
> +/// S instance("string");
> +/// \endcode
> +///
> +/// In the constructor call a string temporary is created from const char * and
> +/// destroyed immediately after the call. This leaves around a dangling
> +/// reference.
> +///
> +/// This check emit warnings for both std::string and ::string const reference
> +/// members.
> +///
> +/// Corresponding cpplint.py check name: 'runtime/member_string_reference'.
> +class StringReferenceMemberCheck : 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_STRING_REF_MEMBER_CHECK_H
>
> Added: clang-tools-extra/trunk/test/clang-tidy/google-member-string-references.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/google-member-string-references.cpp?rev=213133&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/test/clang-tidy/google-member-string-references.cpp (added)
> +++ clang-tools-extra/trunk/test/clang-tidy/google-member-string-references.cpp Wed Jul 16 05:00:14 2014
> @@ -0,0 +1,49 @@
> +// 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;
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits