diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -7,6 +7,7 @@ AvoidConstParamsInDecls.cpp BracesAroundStatementsCheck.cpp ConstReturnTypeCheck.cpp + ContainerDataPointerCheck.cpp ContainerSizeEmptyCheck.cpp ConvertMemberFunctionsToStatic.cpp DeleteNullPointerCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h @@ -0,0 +1,45 @@ +//===--- ContainerDataPointerCheck.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_READABILITY_CONTAINERDATAPOINTERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERDATAPOINTERCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace readability { +/// Checks whether a call to `operator[]` and `&` can be replaced with a call to +/// `data()`. +/// +/// This only replaces the case where the offset being accessed through the +/// subscript operation is a known constant 0. This avoids a potential invalid +/// memory access when the container is empty. Cases where the constant is not +/// explictly zero can be addressed through the clang static analyzer, and those +/// which cannot be statically identified can be caught using UBSan. +class ContainerDataPointerCheck : public ClangTidyCheck { +public: + ContainerDataPointerCheck(StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LO) const override { + return LO.CPlusPlus11; + } + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + + llvm::Optional getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } +}; +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERDATAPOINTERCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp @@ -0,0 +1,117 @@ +//===--- ContainerDataPointerCheck.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 "ContainerDataPointerCheck.h" + +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/StringRef.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { +ContainerDataPointerCheck::ContainerDataPointerCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + +void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) { + const auto Record = + cxxRecordDecl( + isSameOrDerivedFrom( + namedDecl( + has(cxxMethodDecl(isPublic(), hasName("data")).bind("data"))) + .bind("container"))) + .bind("record"); + + const auto NonTemplateContainerType = + qualType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(Record)))); + const auto TemplateContainerType = + qualType(hasUnqualifiedDesugaredType(templateSpecializationType( + hasDeclaration(classTemplateDecl(has(Record)))))); + + const auto Container = + qualType(anyOf(NonTemplateContainerType, TemplateContainerType)); + + Finder->addMatcher( + unaryOperator( + unless(isExpansionInSystemHeader()), hasOperatorName("&"), + hasUnaryOperand(anyOf( + ignoringParenImpCasts( + cxxOperatorCallExpr( + callee(cxxMethodDecl(hasName("operator[]")) + .bind("operator[]")), + argumentCountIs(2), + hasArgument( + 0, + anyOf(ignoringParenImpCasts( + declRefExpr( + to(varDecl(anyOf( + hasType(Container), + hasType(references(Container)))))) + .bind("var")), + ignoringParenImpCasts(hasDescendant( + declRefExpr( + to(varDecl(anyOf( + hasType(Container), + hasType(pointsTo(Container)), + hasType(references(Container)))))) + .bind("var"))))), + hasArgument(1, + ignoringParenImpCasts( + integerLiteral(equals(0)).bind("zero")))) + .bind("operator-call")), + ignoringParenImpCasts( + cxxMemberCallExpr( + hasDescendant( + declRefExpr(to(varDecl(anyOf( + hasType(Container), + hasType(references(Container)))))) + .bind("var")), + argumentCountIs(1), + hasArgument(0, + ignoringParenImpCasts( + integerLiteral(equals(0)).bind("zero")))) + .bind("member-call")), + ignoringParenImpCasts( + arraySubscriptExpr( + hasLHS(ignoringParenImpCasts( + declRefExpr(to(varDecl(anyOf( + hasType(Container), + hasType(references(Container)))))) + .bind("var"))), + hasRHS(ignoringParenImpCasts( + integerLiteral(equals(0)).bind("zero")))) + .bind("array-subscript"))))) + .bind("address-of"), + this); +} + +void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) { + const auto *UO = Result.Nodes.getNodeAs("address-of"); + const auto *DRE = Result.Nodes.getNodeAs("var"); + + std::string ReplacementText; + ReplacementText = std::string(Lexer::getSourceText( + CharSourceRange::getTokenRange(DRE->getSourceRange()), + *Result.SourceManager, getLangOpts())); + if (DRE->getType()->isPointerType()) + ReplacementText += "->data()"; + else + ReplacementText += ".data()"; + + FixItHint Hint = + FixItHint::CreateReplacement(UO->getSourceRange(), ReplacementText); + diag(UO->getBeginLoc(), + "'data' should be used for accessing the data pointer instead of taking " + "the address of the 0-th element") + << Hint; +} +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -12,6 +12,7 @@ #include "AvoidConstParamsInDecls.h" #include "BracesAroundStatementsCheck.h" #include "ConstReturnTypeCheck.h" +#include "ContainerDataPointerCheck.h" #include "ContainerSizeEmptyCheck.h" #include "ConvertMemberFunctionsToStatic.h" #include "DeleteNullPointerCheck.h" @@ -62,6 +63,8 @@ "readability-braces-around-statements"); CheckFactories.registerCheck( "readability-const-return-type"); + CheckFactories.registerCheck( + "readability-container-data-pointer"); CheckFactories.registerCheck( "readability-container-size-empty"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -91,6 +91,11 @@ variables and function parameters only. +- New :doc:`readability-data-pointer +struct vector { + using size_type = size_t; + + vector(); + explicit vector(size_type); + + T *data(); + const T *data() const; + + T &operator[](size_type); + const T &operator[](size_type) const; +}; + +template +struct basic_string { + using size_type = size_t; + + basic_string(); + + T *data(); + const T *data() const; + + T &operator[](size_t); + const T &operator[](size_type) const; +}; + +typedef basic_string string; +typedef basic_string wstring; + +template +struct is_integral; + +template <> +struct is_integral { + static const bool value = true; +}; + +template +struct enable_if { }; + +template +struct enable_if { + typedef T type; +}; +} + +template +void f(const T *); + +#define z (0) + +void g(size_t s) { + std::vector b(s); + f(&((b)[(z)])); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + // CHECK-FIXES: {{^ }}f(b.data());{{$}} +} + +void h() { + std::string s; + f(&((s).operator[]((z)))); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + // CHECK-FIXES: {{^ }}f(s.data());{{$}} + + std::wstring w; + f(&((&(w))->operator[]((z)))); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + // CHECK-FIXES: {{^ }}f(w.data());{{$}} +} + +template ::value>::type> +void i(U s) { + std::vector b(s); + f(&b[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + // CHECK-FIXES: {{^ }}f(b.data());{{$}} +} + +template void i(size_t); + +void j(std::vector * const v) { + f(&(*v)[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + // CHECK-FIXES: {{^ }}f(v->data());{{$}} +} + +void k(const std::vector &v) { + f(&v[0]); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + // CHECK-FIXES: {{^ }}f(v.data());{{$}} +} + +void l() { + unsigned char b[32]; + f(&b[0]); + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] +} + +template +void m(const std::vector &v) { + const T *p = &v[0]; + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:16: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] +}