[clang-tidy] Fix readability-container-data-pointer check

[zeyi2]

Fix issue in readability-container-data-pointer when the container expression is a dereference (e.g., &(*p)[0]). The previous fix-it suggested *p.data(), which changes semantics because . binds tighter than *. The fix now correctly suggests (*p).data().

Closes #164852

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: mitchell (zeyi2)

Changes

Fix issue in readability-container-data-pointer when the container expression is a dereference (e.g., &(*p)[0]). The previous fix-it suggested *p.data(), which changes semantics because . binds tighter than *. The fix now correctly suggests (*p).data().

Closes #164852

---

Full diff: https://github.com/llvm/llvm-project/pull/165636.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp (+5-2)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp (+12)

diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
index 11756d10a8221..d9338888cc40e 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
@@ -107,8 +107,11 @@ void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
       Lexer::getSourceText(CharSourceRange::getTokenRange(SrcRange),
                            *Result.SourceManager, getLangOpts())};
 
-  if (!isa<DeclRefExpr, ArraySubscriptExpr, CXXOperatorCallExpr, CallExpr,
-           MemberExpr>(CE))
+  const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(CE);
+  bool NeedsParens =
+      OpCall ? (OpCall->getOperator() != OO_Subscript)
+             : !isa<DeclRefExpr, MemberExpr, ArraySubscriptExpr, CallExpr>(CE);
+  if (NeedsParens)
     ReplacementText = "(" + ReplacementText + ")";
 
   if (CE->getType()->isPointerType())
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 a8e0eb6d262e6..26f9d9f16ac8c 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
@@ -35,6 +35,12 @@ template <typename T>
 struct enable_if<true, T> {
   typedef T type;
 };
+
+template <typename T>
+struct unique_ptr {
+  T &operator*() const;
+  T *operator->() const;
+};
 }
 
 template <typename T>
@@ -144,3 +150,9 @@ int *r() {
   // 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 holder.v.data();
 }
+
+void s(std::unique_ptr<std::vector<unsigned char>> p) {
+  f(&(*p)[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((*p).data());
+}

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: mitchell (zeyi2)

Changes

Fix issue in readability-container-data-pointer when the container expression is a dereference (e.g., &(*p)[0]). The previous fix-it suggested *p.data(), which changes semantics because . binds tighter than *. The fix now correctly suggests (*p).data().

Closes #164852

---

Full diff: https://github.com/llvm/llvm-project/pull/165636.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp (+5-2)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-data-pointer.cpp (+12)

diff --git a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
index 11756d10a8221..d9338888cc40e 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp
@@ -107,8 +107,11 @@ void ContainerDataPointerCheck::check(const MatchFinder::MatchResult &Result) {
       Lexer::getSourceText(CharSourceRange::getTokenRange(SrcRange),
                            *Result.SourceManager, getLangOpts())};
 
-  if (!isa<DeclRefExpr, ArraySubscriptExpr, CXXOperatorCallExpr, CallExpr,
-           MemberExpr>(CE))
+  const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(CE);
+  bool NeedsParens =
+      OpCall ? (OpCall->getOperator() != OO_Subscript)
+             : !isa<DeclRefExpr, MemberExpr, ArraySubscriptExpr, CallExpr>(CE);
+  if (NeedsParens)
     ReplacementText = "(" + ReplacementText + ")";
 
   if (CE->getType()->isPointerType())
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 a8e0eb6d262e6..26f9d9f16ac8c 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
@@ -35,6 +35,12 @@ template <typename T>
 struct enable_if<true, T> {
   typedef T type;
 };
+
+template <typename T>
+struct unique_ptr {
+  T &operator*() const;
+  T *operator->() const;
+};
 }
 
 template <typename T>
@@ -144,3 +150,9 @@ int *r() {
   // 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 holder.v.data();
 }
+
+void s(std::unique_ptr<std::vector<unsigned char>> p) {
+  f(&(*p)[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((*p).data());
+}

[EugeneZelenko]

Please mention changes in Release Notes.

[vbvictor]

LGTM
You can apply changes from GitHub and could be merged