[PATCH] D115124: [clang-tidy] Fix `readability-container-size-empty` check for smart pointers

[Fabian Wolff via Phabricator via cfe-commits]

fwolff created this revision.
fwolff added reviewers: mizvekov, compnerd.
fwolff added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.

Fixes PR#51776 <https://bugs.llvm.org/show_bug.cgi?id=51776>. If the object on 
which `size()` is called has an overloaded `operator->`, the span for `E` will 
include the `->`, and since the return type of `operator->` is a pointer, 
`readability-container-size-empty` will try to add another arrow, leading to 
the nonsensical `vp->->empty()` suggestion. This patch fixes this behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115124

Files:
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
===================================================================
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
@@ -696,3 +696,17 @@
   instantiatedTemplateWithSizeCall<TypeWithSize>();
   instantiatedTemplateWithSizeCall<std::vector<int>>();
 }
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+  T *operator->() const;
+};
+} // namespace std
+
+bool call_through_unique_ptr(const std::unique_ptr<std::vector<int>> &ptr) {
+  return ptr->size() > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be 
used
+  // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+  // CHECK-FIXES: {{^  }}return !ptr->empty();
+}
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -194,7 +194,13 @@
   if (isBinaryOrTernary(E) || isa<UnaryOperator>(E)) {
     ReplacementText = "(" + ReplacementText + ")";
   }
-  if (E->getType()->isPointerType())
+  const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E);
+  if (OpCallExpr &&
+      OpCallExpr->getOperator() == OverloadedOperatorKind::OO_Arrow) {
+    // This can happen if the object is a smart pointer. Don't add anything
+    // because a '->' is already there (PR#51776), just call the method.
+    ReplacementText += "empty()";
+  } else if (E->getType()->isPointerType())
     ReplacementText += "->empty()";
   else
     ReplacementText += ".empty()";



Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
@@ -696,3 +696,17 @@
   instantiatedTemplateWithSizeCall<TypeWithSize>();
   instantiatedTemplateWithSizeCall<std::vector<int>>();
 }
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+  T *operator->() const;
+};
+} // namespace std
+
+bool call_through_unique_ptr(const std::unique_ptr<std::vector<int>> &ptr) {
+  return ptr->size() > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used
+  // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+  // CHECK-FIXES: {{^  }}return !ptr->empty();
+}
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -194,7 +194,13 @@
   if (isBinaryOrTernary(E) || isa<UnaryOperator>(E)) {
     ReplacementText = "(" + ReplacementText + ")";
   }
-  if (E->getType()->isPointerType())
+  const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E);
+  if (OpCallExpr &&
+      OpCallExpr->getOperator() == OverloadedOperatorKind::OO_Arrow) {
+    // This can happen if the object is a smart pointer. Don't add anything
+    // because a '->' is already there (PR#51776), just call the method.
+    ReplacementText += "empty()";
+  } else if (E->getType()->isPointerType())
     ReplacementText += "->empty()";
   else
     ReplacementText += ".empty()";

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits