Author: Denis Nikitin Date: 2023-01-25T20:06:41Z New Revision: 716469b6139ab5ec5c5b5dac32891300260db8eb
URL: https://github.com/llvm/llvm-project/commit/716469b6139ab5ec5c5b5dac32891300260db8eb DIFF: https://github.com/llvm/llvm-project/commit/716469b6139ab5ec5c5b5dac32891300260db8eb.diff LOG: [clang-tidy] Fix segfault in bugprone-standalone-empty The check incorrectly identified empty() method call in the template class definition as a stand-alone function call. This led to a crash because the checker did not expect empty() function calls without arguments. Fixes: https://github.com/llvm/llvm-project/issues/59487 Reviewed By: cjdb Differential Revision: https://reviews.llvm.org/D142423 Added: Modified: clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp index 67d8e3693fb56..a66f838f1c8fa 100644 --- a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp @@ -29,10 +29,12 @@ namespace clang::tidy::bugprone { using ast_matchers::BoundNodes; using ast_matchers::callee; using ast_matchers::callExpr; +using ast_matchers::classTemplateDecl; using ast_matchers::cxxMemberCallExpr; using ast_matchers::cxxMethodDecl; using ast_matchers::expr; using ast_matchers::functionDecl; +using ast_matchers::hasAncestor; using ast_matchers::hasName; using ast_matchers::hasParent; using ast_matchers::ignoringImplicit; @@ -70,10 +72,13 @@ const Expr *getCondition(const BoundNodes &Nodes, const StringRef NodeId) { } void StandaloneEmptyCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { + // Ignore empty calls in a template definition which fall under callExpr + // non-member matcher even if they are methods. const auto NonMemberMatcher = expr(ignoringImplicit(ignoringParenImpCasts( callExpr( hasParent(stmt(optionally(hasParent(stmtExpr().bind("stexpr")))) .bind("parent")), + unless(hasAncestor(classTemplateDecl())), callee(functionDecl(hasName("empty"), unless(returns(voidType()))))) .bind("empty")))); const auto MemberMatcher = @@ -154,6 +159,8 @@ void StandaloneEmptyCheck::check(const MatchFinder::MatchResult &Result) { return; if (ParentReturnStmt) return; + if (NonMemberCall->getNumArgs() != 1) + return; SourceLocation NonMemberLoc = NonMemberCall->getExprLoc(); SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc(); diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst index 049698a8b0f7a..80805bafddd50 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst @@ -3,8 +3,8 @@ bugprone-standalone-empty ========================= -Warns when `empty()` is used on a range and the result is ignored. Suggests -`clear()` if it is an existing member function. +Warns when ``empty()`` is used on a range and the result is ignored. Suggests +``clear()`` if it is an existing member function. The ``empty()`` method on several common ranges returns a Boolean indicating whether or not the range is empty, but is often mistakenly interpreted as @@ -29,3 +29,8 @@ A call to ``clear()`` would appropriately clear the contents of the range: std::vector<int> v; ... v.clear(); + +Limitations: +- Doesn't warn if ``empty()`` is defined and used with the ignore result in the + class template definition (for example in the library implementation). These + error cases can be caught with ``[[nodiscard]]`` attribute. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp index d7615fdbbe922..53c651879f84b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -80,6 +80,10 @@ template <class T> void empty(T &&); } // namespace test +namespace test_no_args { +bool empty(); +} // namespace test_no_args + namespace base { template <typename T> struct base_vector { @@ -306,6 +310,9 @@ bool test_qualified_empty() { test::empty(v); // no-warning + + test_no_args::empty(); + // no-warning } { @@ -876,3 +883,24 @@ bool test_clear_with_qualifiers() { // no-warning } } + +namespace user_lib { +template <typename T> +struct vector { + bool empty(); + bool test_empty_inside_impl() { + empty(); + // no-warning + return empty(); + // no-warning + } +}; +} // namespace user_lib + +bool test_template_empty_outside_impl() { + user_lib::vector<int> v; + v.empty(); + // CHECK-MESSAGES: :[[#@LINE-1]]:3: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + return v.empty(); + // no-warning +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits