[clang-tidy] don't warn when returning the result for bugprone-standalone-empty

[v1nh1shungry]

Summary

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<int> v;
  return v.empty();
  // will get a warning here, which makes no sense IMO
}

[cjdb]

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.

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

886–890

Please add additional cases similar to what's above (e.g. a case with absl::empty, an ADL-invoked case, etc.).

[v1nh1shungry]

add more tests

[v1nh1shungry]

Thank you for reviewing and giving suggestions! @cjdb

Hope there are enough test cases now, and not too many tests.

[cjdb]

Excellent, let's wait for @denik's feedback before merging, but this LGTM. Thank you for the patch!

[denik]

Thanks @v1nh1shungry for the fix!
The change and test cases look good to me.

[v1nh1shungry]

Thank you for reviewing, @cjdb and @denik! If this patch is okay to land, could you please help me commit it? Thanks a lot!

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

 void StandaloneEmptyCheck::check(const MatchFinder::MatchResult &Result) {
   // Skip if the parent node is Expr.
   if (Result.Nodes.getNodeAs<Expr>("parent"))
     return;
 
   const auto PParentStmtExpr = Result.Nodes.getNodeAs<Expr>("stexpr");
   const auto ParentCompStmt = Result.Nodes.getNodeAs<CompoundStmt>("parent");
   const auto *ParentCond = getCondition(Result.Nodes, "parent");
+  const auto *ParentReturnStmt = Result.Nodes.getNodeAs<ReturnStmt>("parent");
 
   if (const auto *MemberCall =
           Result.Nodes.getNodeAs<CXXMemberCallExpr>("empty")) {
     // Skip if it's a condition of the parent statement.
     if (ParentCond == MemberCall->getExprStmt())
       return;
     // Skip if it's the last statement in the GNU extension
     // statement expression.
     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();
     SourceRange ReplacementRange = SourceRange(ReplacementLoc, ReplacementLoc);
 
     ASTContext &Context = MemberCall->getRecordDecl()->getASTContext();
     DeclarationName Name =
         Context.DeclarationNames.getIdentifier(&Context.Idents.get("clear"));
void StandaloneEmptyCheck::check(const MatchFinder::MatchResult &Result) {
 
   } else if (const auto *NonMemberCall =
                  Result.Nodes.getNodeAs<CallExpr>("empty")) {
     if (ParentCond == NonMemberCall->getExprStmt())
       return;
     if (PParentStmtExpr && ParentCompStmt &&
         ParentCompStmt->body_back() == NonMemberCall->getExprStmt())
       return;
+    if (ParentReturnStmt)
+      return;
 
     SourceLocation NonMemberLoc = NonMemberCall->getExprLoc();
     SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc();
 
     const Expr *Arg = NonMemberCall->getArg(0);
     CXXRecordDecl *ArgRecordDecl = Arg->getType()->getAsCXXRecordDecl();
     if (ArgRecordDecl == nullptr)
       return;

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

struct vector_with_volatile_empty {
   void clear();
 };
 
 template <typename T>
 bool empty(T &&);
 } // namespace qualifiers
 
 
-void test_member_empty() {
+bool test_member_empty() {
   {
     std::vector<int> v;
     v.empty();
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty]
   }
 
   {
     std::vector_with_void_empty<int> v;
bool test_member_empty() {
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty]
   }
 
   {
     absl::string_with_clear_variable s;
     s.empty();
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty]
   }
+
+  {
+    std::vector<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    std::vector_with_int_empty<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear_args<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear_variable<int> v;
+    return v.empty();
+    // no-warning
   }
 
-void test_qualified_empty() {
+  {
+    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
+  }
+}
+
+bool test_qualified_empty() {
   {
     absl::string_with_clear v;
     std::empty(v);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty'; did you mean 'clear()'? [bugprone-standalone-empty]
     // CHECK-FIXES: {{^  }}  v.clear();{{$}}
 
     absl::empty(v);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'absl::empty'; did you mean 'clear()'? [bugprone-standalone-empty]
bool test_qualified_empty() {
   }
 
   {
     std::empty(0);
     // no-warning
     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<int> v;
     empty(v);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty]
   }
 
   {
     std::vector_with_void_empty<int> v;
bool test_unqualified_empty() {
 
   {
     absl::string_with_clear s;
     using absl::empty;
     empty(s);
     // 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<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    std::vector_with_void_empty<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    std::vector_with_int_empty<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear_args<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear_variable<int> 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<int> v;
+    using std::empty;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    std::vector_with_clear<int> 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() {
   std::vector<int> v;
   bool EmptyReturn(v.empty());
   // no-warning
 
   (void)v.empty();
case true:
       std::empty(s);
       // CHECK-MESSAGES: :[[#@LINE-1]]:7: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty]
   }
 
   bool StmtExprReturn = ({std::empty(s); std::empty(s);});
   // 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<int> v;
     v.empty();
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()'; did you mean 'clear()'? [bugprone-standalone-empty]
     // CHECK-FIXES: {{^  }}  v.clear();{{$}}
   }
 
   {
bool test_clear_in_base_class() {
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'base::empty' [bugprone-standalone-empty]
   }
 
   {
     base::vector_clear_variable<int> v;
     empty(v);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'base::empty' [bugprone-standalone-empty]
   }
+
+  {
+    base::vector<int> v;
+    return v.empty();
+    // no-warning
   }
 
-void test_clear_with_qualifiers() {
+  {
+    base::vector_non_dependent<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    base::vector_clear_with_args<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    base::vector_clear_variable<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    base::vector<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    base::vector_non_dependent<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    base::vector_clear_with_args<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    base::vector_clear_variable<int> v;
+    return empty(v);
+    // no-warning
+  }
+}
+
+bool test_clear_with_qualifiers() {
   {
     qualifiers::vector_with_const_clear<int> v;
     v.empty();
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty]
   }
 
   {
     const qualifiers::vector_with_const_clear<int> v;
bool test_clear_with_qualifiers() {
     // CHECK-FIXES: {{^  }}  v.clear();{{$}}
   }
 
   {
     volatile std::vector_with_clear<int> v;
     empty(v);
     // CHECK-MESSAGES: :[[#@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty]
   }
+
+  {
+    qualifiers::vector_with_const_clear<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    const qualifiers::vector_with_const_clear<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    const qualifiers::vector_with_const_empty<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    qualifiers::vector_with_const_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    const qualifiers::vector_with_const_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    const std::vector_with_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    qualifiers::vector_with_volatile_clear<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    volatile qualifiers::vector_with_volatile_clear<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    volatile qualifiers::vector_with_volatile_empty<int> v;
+    return v.empty();
+    // no-warning
+  }
+
+  {
+    qualifiers::vector_with_volatile_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    volatile qualifiers::vector_with_volatile_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
+
+  {
+    volatile std::vector_with_clear<int> v;
+    return empty(v);
+    // no-warning
+  }
 }