[clang-tools-extra] [clang-tidy] Improve `container-data-pointer` check to use `c_str()` (PR #71304)
Eli Black via cfe-commits
cfe-commits at lists.llvm.org
Sun Nov 5 08:05:59 PST 2023
https://github.com/neoncube2 updated https://github.com/llvm/llvm-project/pull/71304
>From ab1e2e7487fdc7b1e16e563c63e225a3dd39da05 Mon Sep 17 00:00:00 2001
From: Eli Black <eliblack3 at hotmail.com>
Date: Sun, 5 Nov 2023 13:43:20 +0800
Subject: [PATCH] [clang-tidy] Improve `container-data-pointer` check to use
`c_str()` instead of `data()` when it's available.
Fixes #55026
---
.../readability/ContainerDataPointerCheck.cpp | 26 ++++++++++++-------
.../readability/ContainerDataPointerCheck.h | 4 +--
clang-tools-extra/docs/ReleaseNotes.rst | 4 +++
.../readability/container-data-pointer.rst | 8 ++----
.../readability/container-data-pointer.cpp | 12 ++++-----
5 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
index a05e228520c9ef1..eb76d05667d300a 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
@@ -40,8 +40,10 @@ void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) {
cxxRecordDecl(
unless(matchers::matchesAnyListedName(IgnoredContainers)),
isSameOrDerivedFrom(
- namedDecl(
- has(cxxMethodDecl(isPublic(), hasName("data")).bind("data")))
+ namedDecl(anyOf(has(cxxMethodDecl(isPublic(), hasName("c_str"))
+ .bind("c_str")),
+ has(cxxMethodDecl(isPublic(), hasName("data"))
+ .bind("data"))))
.bind("container")))
.bind("record");
@@ -93,6 +95,8 @@ void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
const auto *DCE = Result.Nodes.getNodeAs<Expr>(DerefContainerExprName);
const auto *ACE = Result.Nodes.getNodeAs<Expr>(AddrOfContainerExprName);
+ const auto *CStrMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("c_str");
+
if (!UO || !CE)
return;
@@ -111,16 +115,18 @@ void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
MemberExpr>(CE))
ReplacementText = "(" + ReplacementText + ")";
- if (CE->getType()->isPointerType())
- ReplacementText += "->data()";
- else
- ReplacementText += ".data()";
+ ReplacementText += CE->getType()->isPointerType() ? "->" : ".";
+ ReplacementText += CStrMethod ? "c_str()" : "data()";
+
+ std::string Description =
+ CStrMethod
+ ? "'c_str' should be used instead of taking the address of the 0-th "
+ "element"
+ : "'data' should be used for accessing the data pointer instead of "
+ "taking the address of the 0-th element";
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;
+ diag(UO->getBeginLoc(), Description) << Hint;
}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
index 2a15b95095171f1..f5c1a974ff84801 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
@@ -12,8 +12,8 @@
#include "../ClangTidyCheck.h"
namespace clang::tidy::readability {
-/// Checks whether a call to `operator[]` and `&` can be replaced with a call to
-/// `data()`.
+/// Finds cases where code references the address of the element at index 0 in a
+/// container and replaces them with calls to ``data()`` or ``c_str()``.
///
/// This only replaces the case where the offset being accessed through the
/// subscript operation is a known constant 0. This avoids a potential invalid
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ecfb3aa9267f140..6477f2243d31e18 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -238,6 +238,10 @@ Changes in existing checks
Casting to ``void`` no longer suppresses issues by default, control this
behavior with the new `AllowCastToVoid` option.
+- Improved :doc:`container-data-pointer
+ <clang-tidy/checks/readability/container-data-pointer>` check
+ to use ``c_str()`` when it's present on a container.
+
- Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables
<clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables>` check
to ignore ``static`` variables declared within the scope of
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst
index 0d10829ed3c2f9b..8a6b48c58005bf2 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst
@@ -3,13 +3,9 @@
readability-container-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.
+Finds cases where code references the address of the element at index 0 in a container and replaces them with calls to ``data()`` or ``c_str()``.
-This also ensures that in the case that the container is empty, the data pointer
+Using ``data()`` or ``c_str()`` is more readable and ensures that if the container is empty, the data pointer
access does not perform an errant memory access.
Options
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
index 7d8500ed4affe36..a53f303eba1f5d3 100644
--- 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
@@ -52,15 +52,15 @@ void g(size_t s) {
void h() {
std::string s;
f(&((s).operator[]((z))));
- // CHECK-MESSAGES-CLASSIC: :[[@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-CLASSIC: {{^ }}f(s.data());{{$}}
- // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]: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-MESSAGES-CLASSIC: :[[@LINE-1]]:5: warning: 'c_str' should be used instead of taking the address of the 0-th element [readability-container-data-pointer]
+ // CHECK-FIXES-CLASSIC: {{^ }}f(s.c_str());{{$}}
+ // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:5: warning: 'c_str' should be used instead of taking the address of the 0-th element [readability-container-data-pointer]
std::wstring w;
f(&((&(w))->operator[]((z))));
- // CHECK-MESSAGES-CLASSIC: :[[@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-CLASSIC: {{^ }}f(w.data());{{$}}
- // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]: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-MESSAGES-CLASSIC: :[[@LINE-1]]:5: warning: 'c_str' should be used instead of taking the address of the 0-th element [readability-container-data-pointer]
+ // CHECK-FIXES-CLASSIC: {{^ }}f(w.c_str());{{$}}
+ // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:5: warning: 'c_str' should be used instead of taking the address of the 0-th element [readability-container-data-pointer]
}
template <typename T, typename U,
More information about the cfe-commits
mailing list