compnerd updated this revision to Diff 369453.
compnerd marked 2 inline comments as done.
compnerd added a comment.
Update release notes to incorporate feedback.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108893/new/
https://reviews.llvm.org/D108893
Files:
clang-tools-extra/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t
+
+typedef __SIZE_TYPE__ size_t;
+
+namespace std {
+template <typename T>
+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 <typename T>
+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<char> string;
+typedef basic_string<wchar_t> wstring;
+
+template <typename T>
+struct is_integral;
+
+template <>
+struct is_integral<size_t> {
+ static const bool value = true;
+};
+
+template <bool, typename T = void>
+struct enable_if { };
+
+template <typename T>
+struct enable_if<true, T> {
+ typedef T type;
+};
+}
+
+template <typename T>
+void f(const T *);
+
+#define z (0)
+
+void g(size_t s) {
+ std::vector<unsigned char> 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 <typename T, typename U,
+ typename = typename std::enable_if<std::is_integral<U>::value>::type>
+void i(U s) {
+ std::vector<T> 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<unsigned char, size_t>(size_t);
+
+void j(std::vector<unsigned char> * 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<unsigned char> &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]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - readability-data-pointer
+
+readability-data-pointer
+========================
+
+Finds cases where code could use ``data()`` rather than the address of the
+element at index 0 in a container. This pattern is commonly used to materialize
+a pointer to the backing data of a container. ``std::vector`` and
+``std::string`` provide a ``data()`` accessor to retrieve the data pointer which
+should be preferred.
+
+This also ensures that in the case that the container is empty, the data pointer
+access does not perform an errant memory access.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -82,6 +82,11 @@
Reports identifiers whose names are too short. Currently checks local variables and function parameters only.
+- New :doc:`readability-data-pointer <clang-tidy/checks/readability-data-pointer` check.
+
+ Finds cases where code could use ``data()`` rather than the address of the
+ element at index 0 in a container.
+
New check aliases
^^^^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ 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<ConstReturnTypeCheck>(
"readability-const-return-type");
+ CheckFactories.registerCheck<ContainerDataPointerCheck>(
+ "readability-container-data-pointer");
CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
"readability-container-size-empty");
CheckFactories.registerCheck<ConvertMemberFunctionsToStatic>(
Index: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
===================================================================
--- /dev/null
+++ 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 teh 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<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+};
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
@@ -0,0 +1,120 @@
+//===--- 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(pointsTo(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(pointsTo(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(pointsTo(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<UnaryOperator>("address-of");
+ const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("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
Index: clang-tools-extra/clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
