[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
This revision was automatically updated to reflect the committed changes. Closed by commit rG7910ee7d8c6d: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty (authored by v1nh1shungry, committed by cjdb). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 Files: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp === --- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -155,7 +155,7 @@ } // namespace qualifiers -void test_member_empty() { +bool test_member_empty() { { std::vector v; v.empty(); @@ -231,9 +231,69 @@ s.empty(); // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] } + + { +std::vector v; +return v.empty(); +// no-warning + } + + { +std::vector_with_clear v; +return v.empty(); +// no-warning + } + + { +std::vector_with_int_empty v; +return v.empty(); +// no-warning + } + + { +std::vector_with_clear_args v; +return v.empty(); +// no-warning + } + + { +std::vector_with_clear_variable v; +return v.empty(); +// no-warning + } + + { +absl::string s; +return s.empty(); +// no-warning + } + + { +absl::string_with_clear s; +return s.empty(); +// no-warning + } + + { +absl::string_with_int_empty s; +return s.empty(); +// no-warning + } + + { +absl::string_with_clear_args s; +return s.empty(); +// no-warning + } + + { +absl::string_with_clear_variable s; +return s.empty(); +// no-warning + } } -void test_qualified_empty() { +bool test_qualified_empty() { { absl::string_with_clear v; std::empty(v); @@ -260,9 +320,30 @@ absl::empty(nullptr); // no-warning } + + { +absl::string_with_clear s; +return std::empty(s); +// no-warning +return absl::empty(s); +// no-warning + } + + { +absl::string s; +return std::empty(s); +// no-warning + } + + { +return std::empty(0); +// no-warning +return absl::empty(nullptr); +// no-warning + } } -void test_unqualified_empty() { +bool test_unqualified_empty() { { std::vector v; empty(v); @@ -370,6 +451,106 @@ // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'absl::empty'; did you mean 'clear()'? [bugprone-standalone-empty] // CHECK-FIXES: {{^ }} s.clear();{{$}} } + + { +std::vector v; +return empty(v); +// no-warning + } + + { +std::vector_with_void_empty v; +return empty(v); +// no-warning + } + + { +std::vector_with_clear v; +return empty(v); +// no-warning + } + + { +std::vector_with_int_empty v; +return empty(v); +// no-warning + } + + { +std::vector_with_clear_args v; +return empty(v); +// no-warning + } + + { +std::vector_with_clear_variable v; +return empty(v); +// no-warning + } + + { +absl::string s; +return empty(s); +// no-warning + } + + { +absl::string_with_void_empty s; +return empty(s); +// no-warning + } + + { +absl::string_with_clear s; +return empty(s); +// no-warning + } + + { +absl::string_with_int_empty s; +return empty(s); +// no-warning + } + + { +absl::string_with_clear_args s; +return empty(s); +// no-warning + } + + { +absl::string_with_clear_variable s; +return empty(s); +// no-warning + } + + { +std::vector v; +using std::empty; +return empty(v); +// no-warning + } + + { +std::vector_with_clear v; +using std::empty; +return empty(v); +// no-warning + } + + { +absl::string s; +using absl::empty; +return empty(s); +// no-warning + } + + { +absl::string_with_clear s; +using absl::empty; +return empty(s); +// no-warning + } } void test_empty_method_expressions() { @@ -444,8 +625,7 @@ // CHECK-MESSAGES: :[[#@LINE-1]]:27: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] } -void test_clear_in_base_class() { - +bool test_clear_in_base_class() { { base::vector v; v.empty(); @@ -497,9 +677,57 @@ empty(v); // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'base::empty' [bugprone-standalone-empty] } + + { +base::vector v; +return v.empty(); +// no-warning + } + + { +base::vector_non_dependent v; +return v.empty(); +// no-warning + } + + { +base::vector_clear_with_args v; +return v.empty(); +// no-warning + }
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
v1nh1shungry added a comment. Thank you for reviewing, @cjdb and @denik! If this patch is okay to land, could you please help me commit it? Thanks a lot! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
denik accepted this revision. denik added a comment. Thanks @v1nh1shungry for the fix! The change and test cases look good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
cjdb accepted this revision. cjdb added a comment. This revision is now accepted and ready to land. Excellent, let's wait for @denik's feedback before merging, but this LGTM. Thank you for the patch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
v1nh1shungry marked an inline comment as done. v1nh1shungry added a comment. Thank you for reviewing and giving suggestions! @cjdb Hope there are enough test cases now, and not too many tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
v1nh1shungry updated this revision to Diff 487044. v1nh1shungry added a comment. add more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 Files: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp === --- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -155,7 +155,7 @@ } // namespace qualifiers -void test_member_empty() { +bool test_member_empty() { { std::vector v; v.empty(); @@ -231,9 +231,69 @@ s.empty(); // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] } + + { +std::vector v; +return v.empty(); +// no-warning + } + + { +std::vector_with_clear v; +return v.empty(); +// no-warning + } + + { +std::vector_with_int_empty v; +return v.empty(); +// no-warning + } + + { +std::vector_with_clear_args v; +return v.empty(); +// no-warning + } + + { +std::vector_with_clear_variable v; +return v.empty(); +// no-warning + } + + { +absl::string s; +return s.empty(); +// no-warning + } + + { +absl::string_with_clear s; +return s.empty(); +// no-warning + } + + { +absl::string_with_int_empty s; +return s.empty(); +// no-warning + } + + { +absl::string_with_clear_args s; +return s.empty(); +// no-warning + } + + { +absl::string_with_clear_variable s; +return s.empty(); +// no-warning + } } -void test_qualified_empty() { +bool test_qualified_empty() { { absl::string_with_clear v; std::empty(v); @@ -260,9 +320,30 @@ absl::empty(nullptr); // no-warning } + + { +absl::string_with_clear s; +return std::empty(s); +// no-warning +return absl::empty(s); +// no-warning + } + + { +absl::string s; +return std::empty(s); +// no-warning + } + + { +return std::empty(0); +// no-warning +return absl::empty(nullptr); +// no-warning + } } -void test_unqualified_empty() { +bool test_unqualified_empty() { { std::vector v; empty(v); @@ -370,6 +451,106 @@ // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'absl::empty'; did you mean 'clear()'? [bugprone-standalone-empty] // CHECK-FIXES: {{^ }} s.clear();{{$}} } + + { +std::vector v; +return empty(v); +// no-warning + } + + { +std::vector_with_void_empty v; +return empty(v); +// no-warning + } + + { +std::vector_with_clear v; +return empty(v); +// no-warning + } + + { +std::vector_with_int_empty v; +return empty(v); +// no-warning + } + + { +std::vector_with_clear_args v; +return empty(v); +// no-warning + } + + { +std::vector_with_clear_variable v; +return empty(v); +// no-warning + } + + { +absl::string s; +return empty(s); +// no-warning + } + + { +absl::string_with_void_empty s; +return empty(s); +// no-warning + } + + { +absl::string_with_clear s; +return empty(s); +// no-warning + } + + { +absl::string_with_int_empty s; +return empty(s); +// no-warning + } + + { +absl::string_with_clear_args s; +return empty(s); +// no-warning + } + + { +absl::string_with_clear_variable s; +return empty(s); +// no-warning + } + + { +std::vector v; +using std::empty; +return empty(v); +// no-warning + } + + { +std::vector_with_clear v; +using std::empty; +return empty(v); +// no-warning + } + + { +absl::string s; +using absl::empty; +return empty(s); +// no-warning + } + + { +absl::string_with_clear s; +using absl::empty; +return empty(s); +// no-warning + } } void test_empty_method_expressions() { @@ -444,8 +625,7 @@ // CHECK-MESSAGES: :[[#@LINE-1]]:27: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] } -void test_clear_in_base_class() { - +bool test_clear_in_base_class() { { base::vector v; v.empty(); @@ -497,9 +677,57 @@ empty(v); // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'base::empty' [bugprone-standalone-empty] } + + { +base::vector v; +return v.empty(); +// no-warning + } + + { +base::vector_non_dependent v; +return v.empty(); +// no-warning + } + + { +base::vector_clear_with_args v; +return v.empty(); +// no-warning + } + + { +base::vector_clear_variable v; +return v.empty(); +// no-warning + } + + { +base::vector v; +return empty(v
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
cjdb added a comment. LGTM, thanks for fixing! Please be sure to have your commit message have `Fixes #59517` instead of a link to the issue, as this will close the bug upon merging. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp:586-590 + { +std::vector v; +return std::empty(v); +// no-warning + } Please add additional cases similar to what's above (e.g. a case with `absl::empty`, an ADL-invoked case, etc.). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141107/new/ https://reviews.llvm.org/D141107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D141107: [clang-tidy] don't warn when returning the result for bugprone-standalone-empty
v1nh1shungry created this revision. v1nh1shungry added reviewers: cjdb, hokein. Herald added subscribers: carlosgalvezp, xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. v1nh1shungry requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Relevant issue: https://github.com/llvm/llvm-project/issues/59517 Currently this check will warn when the result is used in a `return` statement, e.g. bool foobar() { std::vector v; return v.empty(); // will get a warning here, which makes no sense IMO } Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D141107 Files: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp === --- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -576,3 +576,16 @@ // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] } } + +bool test_empty_in_return() { + { +std::vector v; +return v.empty(); +// no-warning + } + { +std::vector v; +return std::empty(v); +// no-warning + } +} Index: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp === --- clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp +++ clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp @@ -98,6 +98,7 @@ const auto PParentStmtExpr = Result.Nodes.getNodeAs("stexpr"); const auto ParentCompStmt = Result.Nodes.getNodeAs("parent"); const auto *ParentCond = getCondition(Result.Nodes, "parent"); + const auto *ParentReturnStmt = Result.Nodes.getNodeAs("parent"); if (const auto *MemberCall = Result.Nodes.getNodeAs("empty")) { @@ -109,6 +110,9 @@ if (PParentStmtExpr && ParentCompStmt && ParentCompStmt->body_back() == MemberCall->getExprStmt()) return; +// Skip if it's a return statement +if (ParentReturnStmt) + return; SourceLocation MemberLoc = MemberCall->getBeginLoc(); SourceLocation ReplacementLoc = MemberCall->getExprLoc(); @@ -150,6 +154,8 @@ if (PParentStmtExpr && ParentCompStmt && ParentCompStmt->body_back() == NonMemberCall->getExprStmt()) return; +if (ParentReturnStmt) + return; SourceLocation NonMemberLoc = NonMemberCall->getExprLoc(); SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc(); Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp === --- clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -576,3 +576,16 @@ // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] } } + +bool test_empty_in_return() { + { +std::vector v; +return v.empty(); +// no-warning + } + { +std::vector v; +return std::empty(v); +// no-warning + } +} Index: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp === --- clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp +++ clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp @@ -98,6 +98,7 @@ const auto PParentStmtExpr = Result.Nodes.getNodeAs("stexpr"); const auto ParentCompStmt = Result.Nodes.getNodeAs("parent"); const auto *ParentCond = getCondition(Result.Nodes, "parent"); + const auto *ParentReturnStmt = Result.Nodes.getNodeAs("parent"); if (const auto *MemberCall = Result.Nodes.getNodeAs("empty")) { @@ -109,6 +110,9 @@ if (PParentStmtExpr && ParentCompStmt && ParentCompStmt->body_back() == MemberCall->getExprStmt()) return; +// Skip if it's a return statement +if (ParentReturnStmt) + return; SourceLocation MemberLoc = MemberCall->getBeginLoc(); SourceLocation ReplacementLoc = MemberCall->getExprLoc(); @@ -150,6 +154,8 @@ if (PParentStmtExpr && ParentCompStmt && ParentCompStmt->body_back() == NonMemberCall->getExprStmt()) return; +if (ParentReturnStmt) + return; SourceLocation NonMemberLoc = NonMemberCall->getExprLoc(); SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc(); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits