[clang-tidy] readability-redundant-smartptr-get: disable for smart pointers to arrays

[FabianWolff]

Currently we generate an incorrect suggestion for shared/unique pointers to arrays; for instance (Godbolt):

#include <memory>

void test_shared_ptr_to_array() {
  std::shared_ptr<int[]> i;
  auto s = sizeof(*i.get());
}

<source>:5:20: warning: redundant get() call on smart pointer [readability-redundant-smartptr-get]
    5 |   auto s = sizeof(*i.get());
      |                    ^~~~~~~
      |                    i
1 warning generated.

sizeof(*i) is incorrect, though, because the array specialization of std::shared/unique_ptr does not have an operator*(). Therefore I have disabled this check for smart pointers to arrays for now; future work could, of course, improve on this by suggesting, say, sizeof(i[0]) in the above example.

[llvmbot]

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

@llvm/pr-subscribers-clang-tidy

Author: None (FabianWolff)

Changes

Currently we generate an incorrect suggestion for shared/unique pointers to arrays; for instance (Godbolt):

#include <memory>

void test_shared_ptr_to_array() {
  std::shared_ptr<int[]> i;
  auto s = sizeof(*i.get());
}

<source>:5:20: warning: redundant get() call on smart pointer [readability-redundant-smartptr-get]
    5 |   auto s = sizeof(*i.get());
      |                    ^~~~~~~
      |                    i
1 warning generated.

sizeof(*i) is incorrect, though, because the array specialization of std::shared/unique_ptr does not have an operator*(). Therefore I have disabled this check for smart pointers to arrays for now; future work could, of course, improve on this by suggesting, say, sizeof(i[0]) in the above example.

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp (+4-1)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get-msvc.cpp (+16)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get.cpp (+15)

diff --git a/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
index be52af77ae0a5..a31489a388fff 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
@@ -44,7 +44,10 @@ internal::Matcher<Expr> callToGet(const internal::Matcher<Decl> &OnClass) {
 }
 
 internal::Matcher<Decl> knownSmartptr() {
-  return recordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"));
+  return recordDecl(
+      hasAnyName("::std::unique_ptr", "::std::shared_ptr"),
+      unless(anyOf(has(cxxMethodDecl(hasName("operator[]"))),
+                   has(functionTemplateDecl(hasName("operator[]"))))));
 }
 
 void registerMatchersForGetArrowStart(MatchFinder *Finder,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get-msvc.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get-msvc.cpp
index bd8990a27b263..3b8e474c6fe8f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get-msvc.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get-msvc.cpp
@@ -26,6 +26,14 @@ struct shared_ptr {
   explicit operator bool() const noexcept;
 };
 
+template <typename T>
+struct shared_ptr<T[]> {
+  template <typename T2 = T>
+  T2* operator[](unsigned) const;
+  T* get() const;
+  explicit operator bool() const noexcept;
+};
+
 }  // namespace std
 
 struct Bar {
@@ -92,3 +100,11 @@ void Positive() {
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
 }
+
+void test_shared_ptr_to_array() {
+  std::shared_ptr<int[]> i;
+  // The array specialization does not have operator*(), so make sure
+  // we do not incorrectly suggest sizeof(*i) here.
+  // FIXME: alternatively, we could suggest sizeof(i[0])
+  auto s = sizeof(*i.get());
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get.cpp
index ec4ca4cb79484..6abe5d7e4e123 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get.cpp
@@ -12,6 +12,13 @@ struct unique_ptr {
   explicit operator bool() const noexcept;
 };
 
+template <typename T>
+struct unique_ptr<T[]> {
+  T& operator[](unsigned) const;
+  T* get() const;
+  explicit operator bool() const noexcept;
+};
+
 template <typename T>
 struct shared_ptr {
   T& operator*() const;
@@ -283,3 +290,11 @@ void test_redundant_get_with_member() {
     // CHECK-FIXES: f(**i->get()->getValue());
  }
 }
+
+void test_unique_ptr_to_array() {
+  std::unique_ptr<int[]> i;
+  // The array specialization does not have operator*(), so make sure
+  // we do not incorrectly suggest sizeof(*i) here.
+  // FIXME: alternatively, we could suggest sizeof(i[0])
+  auto s = sizeof(*i.get());
+}

[kadircet]

thanks a lot for taking a lookg here!

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get-msvc.cpp clang-tools-extra/test/clang-tidy/checkers/readability/redundant-smartptr-get.cpp

View the diff from clang-format here.

diff --git a/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
index baa977750..9774d93ff 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
@@ -66,8 +66,8 @@ void registerMatchersForGetArrowStart(MatchFinder *Finder,
 
   // Make sure we are not missing the known standard types.
   const auto SmartptrAny = anyOf(knownSmartptr(), QuacksLikeASmartptr);
-  const auto SmartptrWithDeref =
-      anyOf(cxxRecordDecl(knownSmartptr(), HasRelevantOps), QuacksLikeASmartptr);
+  const auto SmartptrWithDeref = anyOf(
+      cxxRecordDecl(knownSmartptr(), HasRelevantOps), QuacksLikeASmartptr);
 
   // Catch 'ptr.get()->Foo()'
   Finder->addMatcher(

[kadircet]

thanks a lot!

[vbvictor]

@kadircet, I think we generally shouldn't merge code that doesn't pass clang-format build (if non-compliant changes are so small). In the future I plan to make whole clang-tidy directory clang-format-complaint so that people can have "format on save: file" in their editors when developing clang-tidy.

Also, an entry in ReleaseNotes.rst about this change is missing. (Did not see it at first look, sorry)
I'd highly appreciate a new PR with entry in release notes and fixed formatting.