llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Eli Black (neoncube2) <details> <summary>Changes</summary> Improve clang-tidy's `container-data-pointer` check to use `c_str()`, when available. Fixes #<!-- -->55026 This is my first Clang PR! :) --- Full diff: https://github.com/llvm/llvm-project/pull/71304.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp (+16-10) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang-tools-extra/docs/clang-tidy/checks/readability/container-data-pointer.rst (+2-6) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp (+6-6) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp index a05e228520c9ef1..c20050063338281 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 != NULL ? "c_str()" : "data()"; + + std::string Description = + CStrMethod != NULL + ? "'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/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, `````````` </details> https://github.com/llvm/llvm-project/pull/71304 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits