fwolff updated this revision to Diff 400625.
fwolff added a comment.
In D115124#3209199 <https://reviews.llvm.org/D115124#3209199>, @Sockke wrote:
> Could you please add a test case where the smart pointer object is
> dereferenced before calling `size()`? E.g. `return (*ptr).size() == 0;`. I
> think this change doesn't work on this test case.
This is not entirely true; what you're seeing is an existing, unrelated bug:
https://godbolt.org/z/9zEfdrPW8
In any case, I've fixed it, added a test for it, and added a bullet point to
the release notes.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115124/new/
https://reviews.llvm.org/D115124
Files:
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
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,25 @@
instantiatedTemplateWithSizeCall<TypeWithSize>();
instantiatedTemplateWithSizeCall<std::vector<int>>();
}
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+ T *operator->() const;
+ 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();
+}
+
+bool call_through_unique_ptr_deref(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/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -83,6 +83,9 @@
- Generalized the `modernize-use-default-member-init` check to handle
non-default
constructors.
+- Fixed incorrect suggestions for `readability-container-size-empty` when
+ smart pointers are involved.
+
New checks
^^^^^^^^^^
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
@@ -191,10 +191,17 @@
std::string ReplacementText = std::string(
Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
*Result.SourceManager, getLangOpts()));
- if (isBinaryOrTernary(E) || isa<UnaryOperator>(E)) {
+ const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E);
+ if (isBinaryOrTernary(E) || isa<UnaryOperator>(E) ||
+ (OpCallExpr && (OpCallExpr->getOperator() == OO_Star))) {
ReplacementText = "(" + ReplacementText + ")";
}
- if (E->getType()->isPointerType())
+ 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,25 @@
instantiatedTemplateWithSizeCall<TypeWithSize>();
instantiatedTemplateWithSizeCall<std::vector<int>>();
}
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+ T *operator->() const;
+ 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();
+}
+
+bool call_through_unique_ptr_deref(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/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -83,6 +83,9 @@
- Generalized the `modernize-use-default-member-init` check to handle non-default
constructors.
+- Fixed incorrect suggestions for `readability-container-size-empty` when
+ smart pointers are involved.
+
New checks
^^^^^^^^^^
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
@@ -191,10 +191,17 @@
std::string ReplacementText = std::string(
Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()),
*Result.SourceManager, getLangOpts()));
- if (isBinaryOrTernary(E) || isa<UnaryOperator>(E)) {
+ const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E);
+ if (isBinaryOrTernary(E) || isa<UnaryOperator>(E) ||
+ (OpCallExpr && (OpCallExpr->getOperator() == OO_Star))) {
ReplacementText = "(" + ReplacementText + ")";
}
- if (E->getType()->isPointerType())
+ 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
