Revert "Re-Revert "clang-tidy: introduce readability-containter-data-pointer check""
authorSaleem Abdulrasool <compnerd@compnerd.org>
Wed, 15 Sep 2021 20:07:51 +0000 (20:07 +0000)
committerSaleem Abdulrasool <compnerd@compnerd.org>
Wed, 15 Sep 2021 20:52:55 +0000 (20:52 +0000)
This reverts commit 626586fc253c6f032aedb325dba6b1ff3f11875e.

Tweak the test for Windows.  Windows defaults to delayed template
parsing, which resulted in the main template definition not registering
the test on Windows.  Process the file with the additional
`-fno-delayed-template-parsing` flag to change the default beahviour.
Additionally, add an extra check for the fix it and use a more robust
test to ensure that the value is always evaluated.

Differential Revision: https://reviews.llvm.org/D108893

clang-tools-extra/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp [new file with mode: 0644]
clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h [new file with mode: 0644]
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 [new file with mode: 0644]
clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp [new file with mode: 0644]

index 78256d6..eba0ab9 100644 (file)
@@ -7,6 +7,7 @@ add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
+  ContainerDataPointerCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
new file mode 100644 (file)
index 0000000..3a67050
--- /dev/null
@@ -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<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
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
new file mode 100644 (file)
index 0000000..0f0f823
--- /dev/null
@@ -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<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERDATAPOINTERCHECK_H
index 366541a..2d65402 100644 (file)
@@ -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 @@ public:
         "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 609064f..1b1f00d 100644 (file)
@@ -91,6 +91,11 @@ New checks
   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
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst
new file mode 100644 (file)
index 0000000..46febd2
--- /dev/null
@@ -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.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp
new file mode 100644 (file)
index 0000000..2c9a758
--- /dev/null
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy %s readability-container-data-pointer %t -- -- -fno-delayed-template-parsing
+
+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]
+}
+
+template <typename T>
+void m(const std::vector<T> &v) {
+  return &v[0];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: 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: {{^  }}return v.data();{{$}}
+}