llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Gaurav Dhingra (gxyd) <details> <summary>Changes</summary> ## Description Fixes #<!-- -->55026 I took inspiration from earlier closed PR: https://github.com/llvm/llvm-project/pull/71304, which guided me to a good solution here. With an aim to get my first PR into the llvm-project, kindly let me know if this needs more work. --- Full diff: https://github.com/llvm/llvm-project/pull/190590.diff 2 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp (+56-11) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp (+26) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp index 6dfef3aa247ef..9d304fd2a8478 100644 --- a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp @@ -38,8 +38,9 @@ void ContainerDataPointerCheck::registerMatchers(MatchFinder *Finder) { cxxRecordDecl( unless(matchers::matchesAnyListedRegexName(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("container"))) .bind("record"); @@ -91,6 +92,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 *CStrDecl = Result.Nodes.getNodeAs<CXXMethodDecl>("c_str"); + if (!UO || !CE) return; @@ -99,6 +102,46 @@ void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) { else if (ACE) CE = ACE; + SourceRange ReplacementRange = UO->getSourceRange(); + bool UseCStr = false; + if (CStrDecl) { + auto Parents = Result.Context->getParents(*UO); + + if (!Parents.empty()) { + if (const auto *VD = Parents[0].get<VarDecl>()) { + QualType VarType = VD->getType(); + if (VarType->isPointerType()) { + QualType PointeeType = VarType->getPointeeType(); + UseCStr = PointeeType.isConstQualified(); + } + } else if (const auto *ICE = Parents[0].get<ImplicitCastExpr>()) { + QualType CastType = ICE->getType(); + if (CastType->isPointerType()) { + QualType PointeeType = CastType->getPointeeType(); + UseCStr = PointeeType.isConstQualified(); + } + } else if (const auto *Cast = Parents[0].get<CastExpr>()) { + QualType CastType = Cast->getType(); + if (CastType->isPointerType()) { + QualType PointeeType = CastType->getPointeeType(); + UseCStr = PointeeType.isConstQualified(); + if (UseCStr) { + // if it's a const cast, use the Cast range as replacement range + // e.g. (const char*)&s[0] -> s.c_str() + ReplacementRange = Cast->getSourceRange(); + } + } + } + } + + if (!UseCStr) { + QualType ContainerType = CE->getType(); + if (ContainerType->isPointerType()) + ContainerType = ContainerType->getPointeeType(); + UseCStr = ContainerType.isConstQualified(); + } + } + const SourceRange SrcRange = CE->getSourceRange(); std::string ReplacementText{ @@ -112,16 +155,18 @@ void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) { if (NeedsParens) ReplacementText = "(" + ReplacementText + ")"; - if (CE->getType()->isPointerType()) - ReplacementText += "->data()"; - else - ReplacementText += ".data()"; + ReplacementText += CE->getType()->isPointerType() ? "->" : "."; + ReplacementText += UseCStr ? "c_str()" : "data()"; + llvm::StringRef Description = UseCStr + ? "'c_str' should be used for accessing " + "the data pointer 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"; const 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; + FixItHint::CreateReplacement(ReplacementRange, ReplacementText); + diag(UO->getBeginLoc(), Description) << Hint; } } // namespace clang::tidy::readability 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 4fd228a554d7d..3efa7ed4501bd 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 @@ -25,11 +25,37 @@ void h() { // 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] + char* p1 = &s[0]; + // CHECK-MESSAGES-CLASSIC: :[[@LINE-1]]:14: 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: char* p1 = s.data(); + // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:14: warning: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + + const char* p2 = &s[0]; + // CHECK-MESSAGES-CLASSIC: :[[@LINE-1]]:20: warning: 'c_str' 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: const char* p2 = s.c_str(); + // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:20: warning: 'c_str' should be used for accessing the data pointer 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] + + f((const char*)&((s).operator[]((z)))); + // CHECK-MESSAGES-CLASSIC: :[[@LINE-1]]:18: warning: 'c_str' 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.c_str()); + // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:18: warning: 'c_str' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + + const std::string cs; + f(&((&(cs))->operator[]((z)))); + // CHECK-MESSAGES-CLASSIC: :[[@LINE-1]]:5: warning: 'c_str' 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(cs.c_str()); + // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:5: warning: 'c_str' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer] + + const char* p3 = &cs[0]; + // CHECK-MESSAGES-CLASSIC: :[[@LINE-1]]:20: warning: 'c_str' 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: const char* p3 = cs.c_str(); + // CHECK-MESSAGES-WITH-CONFIG-NOT: :[[@LINE-3]]:20: warning: 'c_str' 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, typename U, `````````` </details> https://github.com/llvm/llvm-project/pull/190590 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
