clang-tidy: introduce readability-containter-data-pointer check

[compnerd]

Summary

This introduces a new check, readability-containter-data-pointer. This
check is meant to catch the cases where the user may be trying to
materialize the data pointer by taking the address of the 0-th member of
a container. With C++11 or newer, the data member should be used for
this. This provides the following benefits:

• .data() is easier to read than &[0]
• it avoids an unnecessary re-materialization of the pointer
  • this doesn't matter in the case of optimized code, but in the case of unoptimized code, this will be visible
• it avoids a potential invalid memory de-reference caused by the indexing when the container is empty (in debug mode, clang will normally optimize away the re-materialization in optimized builds).

The small potential behavioural change raises the question of where the
check should belong. A reasoning of defense in depth applies here, and
this does an unchecked conversion, with the assumption that users can
use the static analyzer to catch cases where we can statically identify
an invalid memory de-reference. For the cases where the static analysis
is unable to prove the size of the container, UBSan can be used to track
the invalid access.

Special thanks to Aaron Ballman for the discussion on whether this
check would be useful and where to place it.

This partially resolves PR26817!

[compnerd]

Reflow the text using clang-format.

[Eugene.Zelenko]

Thank you for implementing 26817! But shouldn't this check belong to modernize module?

Please add documentation and mention new check in Release Notes.

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

19

Please separate with empty line.

118

Please separate with empty line.

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h

18

Please separate with empty line.

41

Please separate with empty line.

46

Please add inclusion guard in comment.

[compnerd]

In D108893#2971410, @Eugene.Zelenko wrote:

Thank you for implementing 26817! But shouldn't this check belong to modernize module?

Oh, I was unaware of the PR, I'll tag that in the commit message, thanks!

Hmm, I'm somewhat on the fence, but I wouldn't be against the reorganization to modernize, only I would prefer that we get that settled before I do the actual rename/move.

Please add documentation and mention new check in Release Notes.

Ah, good idea, I'll add that as well.

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

19

Hmm, this is what clang-format does, which should be unambiguously correct. Is there something that needs to be changed in clang-format or .clang-format?

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

19

Hard to tell about current state of clang-format, but you could take a look on existing checks code.

[compnerd]

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

19

I would prefer that we fix the clang-format configuration rather than manually change the formatting here. This is introducing a new set of files, so it isn't resulting in inconsistency in a single file. I don't mind reformatting the code with clang-format, but generally, that is considered the definitive arbiter for formatting.

[compnerd]

Hmm, one case that doesn't currently get handled properly is the following test case:

template <typename T> void f(const T *);
void g(const std::vector<unsigned char> **v) {
  f(&(**v)[0]);
}

[compnerd]

Add some documentation and release notes.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

96

It'll be good idea to mention containers and that data is method.

clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst

7

Please make first statement same as in Release Notes.

[compnerd]

clang-tools-extra/docs/ReleaseNotes.rst

96

That was dumb, you're right. Thanks!

clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst

7

That was a good idea. I find that the updated text reads much better too.

[compnerd]

Update release notes to incorporate feedback.

[compnerd]

@Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and assuming that he's okay with modernize.container-data-pointer, I'll change it to that.

[Eugene.Zelenko]

In D108893#2972634, @compnerd wrote:

@Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and assuming that he's okay with modernize.container-data-pointer, I'll change it to that.

Sure, Aaron has much broader vision than me.

[aaron.ballman]

In D108893#2972651, @Eugene.Zelenko wrote:

In D108893#2972634, @compnerd wrote:

@Eugene.Zelenko I'd like to have @aaron.ballman weigh in on the naming, and assuming that he's okay with modernize.container-data-pointer, I'll change it to that.

Sure, Aaron has much broader vision than me.

Lies! I just wear glasses, that's all. ;-)

I can see a case being made for either modernize or readability. For modernize, data() was newly added in C++11, so you could argue that this is used to modernize C++98 code. For readability, data() makes the semantics of the underlying code more clear. Also, bugprone makes sense too because doing &v[0] is UB if v is empty, but v.data() is not (immediately) UB.

I think bugprone would be a tough place to put it because of false positive concerns. This is more of a blanket code rewriting utility than it is trying to catch bugs (the static analyzer and UBSan are better tools for that in that case).
I think modernize would be reasonable, but given that C++11 is already over ten years old, I feel like this is losing its "modernization" aspects in some ways. Also, this only covers converting &vec[0] into a call to data(); for modernize, I'd expect it to convert &vec[N] into vec.data() + N more broadly.
I think readability makes slightly more sense because then we can position this as a rewriting tool to make the code more readable (so there are no false positives -- it transforms all &vec[0] calls to data()).

However, if someone wants to make a strong case for modernize, I think it'd be defensible. Also, if someone wanted to suggest we add it to both readability and modernize, that could also make sense (perhaps with an option to convert &vec[N] that's on by default for modernize and off by default for readability).

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

57

Is this pointsTo() correct? IIRC, that would be trying to capture something like:

std::vector<int> *container;
&container[0];

but this wouldn't call the operator[] overload, it'd be using the builtin array subscripting, so I'm not certain this would match anything.

64

Same question here about pointsTo().

76

Here too.

89

And here.

103–106

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h

24

46

[compnerd]

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

64

This is actually meant to catch cases such as references to UnOp(DRE).

[compnerd]

Address review feedback

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

64

Good catch and thank you for all the off-list discussion on this point!

clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

105

Can you add one more test for how this behaves on uninstantiated templates?

template <typename Ty>
void func(const std::vector<Ty> &Vec) {
  const Ty *Ptr = &Vec[0]; // Should diagnose and provide a fix-it despite not instantiating func()
}

[compnerd]

clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

105

Absolutely! Thanks for the additional test case.

[aaron.ballman]

LGTM modulo the testing request (I forgot to mention that when marking it yesterday, oops!).

[Eugene.Zelenko]

What about adding modernize/bugprone aliases?

[aaron.ballman]

In D108893#2999645, @Eugene.Zelenko wrote:

What about adding modernize/bugprone aliases?

I'd be fine if we wanted to add aliases, but I'd sort of expect some extra functionality out of a check in those modules. I think it's fine to land this now, and we can add aliases in a follow-up. WDYT?

[Eugene.Zelenko]

In D108893#2999654, @aaron.ballman wrote:

In D108893#2999645, @Eugene.Zelenko wrote:

What about adding modernize/bugprone aliases?

I'd be fine if we wanted to add aliases, but I'd sort of expect some extra functionality out of a check in those modules. I think it's fine to land this now, and we can add aliases in a follow-up. WDYT?

Aliases code is trivial comparing with check itself, so it make sense to add it in this patch.

[aaron.ballman]

In D108893#2999681, @Eugene.Zelenko wrote:

In D108893#2999654, @aaron.ballman wrote:

In D108893#2999645, @Eugene.Zelenko wrote:

What about adding modernize/bugprone aliases?

I'd be fine if we wanted to add aliases, but I'd sort of expect some extra functionality out of a check in those modules. I think it's fine to land this now, and we can add aliases in a follow-up. WDYT?

Aliases code is trivial comparing with check itself, so it make sense to add it in this patch.

So you'd like to see that extra functionality added now? (I don't think it makes sense to have the check as-is in both readability and modernize.)

[Eugene.Zelenko]

So you'd like to see that extra functionality added now? (I don't think it makes sense to have the check as-is in both readability and modernize.)

There are plenty of other cross-module aliases already.

[aaron.ballman]

In D108893#2999698, @Eugene.Zelenko wrote:

So you'd like to see that extra functionality added now? (I don't think it makes sense to have the check as-is in both readability and modernize.)

There are plenty of other cross-module aliases already.

Those are most often checks shared with coding guidelines or they have configurable differences between the modules. I do not think we commonly share identical checks between modules that are not coding standards.

[compnerd]

@Eugene.Zelenko - sorry, I didn't see the additional comments before the commit. I'm happy to do a follow up depending on the resolution.

[aaron.ballman]

In D108893#2999763, @compnerd wrote:

@Eugene.Zelenko - sorry, I didn't see the additional comments before the commit. I'm happy to do a follow up depending on the resolution.

If there's a follow-up to add this to modernize, I would say that version of the check should transition &v[N] to v.data() + N (perhaps optionally). I don't think a follow-up to add this to bugprone makes a lot of sense because of the high potential for false positive diagnostics (I think the bugprone one should be a static analyzer check that looks for a preceding predicate that checks the size of the container or whether it's empty).

[thakis]

This breaks tests: http://45.33.8.238/linux/55784/step_8.txt

Please take a look and revert for now if it takes a while to fix.

[thakis]

Reverted in 76dc8ac36d07 for now.

[compnerd]

@thakis - thanks, seems that I had a part of the change sitting in my stash ... I had added a -NOT to verify the behaviour, and forgot to remove it. I'll revert the revert with the fix.

[MaskRay]

49992c04148e5327bef9bd2dff53a0d46004b4b4 relanded the change. It is useful to attach the original Differential Revision: so that it is connected to this Differential.

[thakis]

The test is still failing on Windows, see e.g. http://45.33.8.238/win/45205/step_8.txt or https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8836086236638553313/+/u/package_clang/stdout?format=raw or https://lab.llvm.org/buildbot/#/builders/123/builds/6072

[steakhal]

It seems like the checker is documented as readability-data-pointer but in the tests it reports issues under the readability-container-data-pointer name.
Shouldn't they be the same? I think it will confuse the users.
@aaron.ballman @whisperity @compnerd

[whisperity]

In D108893#3111674, @steakhal wrote:

It seems like the checker is documented as readability-data-pointer but in the tests it reports issues under the readability-container-data-pointer name.
Shouldn't they be the same? I think it will confuse the users.
@aaron.ballman @whisperity @compnerd

Sounds like a typo or an intermittent rebase or rename missing something. Definitely should be consistent.

[whisperity]

In D108893#3118389, @whisperity wrote:

In D108893#3111674, @steakhal wrote:

It seems like the checker is documented as readability-data-pointer but in the tests it reports issues under the readability-container-data-pointer name.
Shouldn't they be the same? I think it will confuse the users.

Sounds like a typo or an intermittent rebase or rename missing something. Definitely should be consistent.

@steakhal Fixed in rG164ee457a04daf727ead99b43ad490ea05523652.

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
+  ContainerDataPointerCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ConvertMemberFunctionsToStatic.cpp
   DeleteNullPointerCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionCognitiveComplexityCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierLengthCheck.cpp
   IdentifierNamingCheck.cpp

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h

+//===--- 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

clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp

+//===--- 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

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
+#include "ContainerDataPointerCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ConvertMemberFunctionsToStatic.h"
 #include "DeleteNullPointerCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionCognitiveComplexityCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierLengthCheck.h"
 #include "IdentifierNamingCheck.h"

 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "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>(
         "readability-convert-member-functions-to-static");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.
 
   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
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-exp42-c
   <clang-tidy/checks/cert-exp42-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/readability-data-pointer.rst

+.. 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.

clang-tools-extra/test/clang-tidy/checkers/readability-container-data-pointer.cpp

+// 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]
+}
+
+template <typename T>
+void m(const std::vector<T> &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]
+}