diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -48,6 +48,7 @@ #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" +#include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" @@ -156,6 +157,8 @@ "bugprone-sizeof-expression"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); + CheckFactories.registerCheck( + "bugprone-standalone-empty"); CheckFactories.registerCheck( "bugprone-string-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -44,6 +44,7 @@ SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp + StandaloneEmptyCheck.cpp StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h @@ -0,0 +1,38 @@ +//===--- StandaloneEmptyCheck.h - clang-tidy --------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STANDALONEEMPTYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STANDALONEEMPTYCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Checks for ignored calls to `empty()` on a range and suggests `clear()` +/// as an alternative if it is an existing member function. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/standalone-empty.html +class StandaloneEmptyCheck : public ClangTidyCheck { +public: + StandaloneEmptyCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STANDALONEEMPTYCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp @@ -0,0 +1,106 @@ +//===--- StandaloneEmptyCheck.cpp - clang-tidy ----------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "StandaloneEmptyCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/AST/DeclBase.h" +#include "clang/AST/DeclCXX.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +void StandaloneEmptyCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { + using namespace ast_matchers; + const auto NonMemberMatcher = + callExpr( + // unless(hasParent(stmt())), + callee(functionDecl(hasName("empty"), unless(returns(voidType()))))) + .bind("empty"); + const auto MemberMatcher = + expr(ignoringImplicit(ignoringParenImpCasts( cxxMemberCallExpr(hasParent(stmtExpr()),callee( + cxxMethodDecl(hasName("empty"), unless(returns(voidType())))))))) + .bind("empty"); + + Finder->addMatcher(MemberMatcher, this); + Finder->addMatcher(NonMemberMatcher, this); +} + +void StandaloneEmptyCheck::check( + const ast_matchers::MatchFinder::MatchResult &Result) { + if (const auto *MemberCall = + Result.Nodes.getNodeAs("empty")) { + SourceLocation MemberLoc = MemberCall->getBeginLoc(); + SourceLocation ReplacementLoc = MemberCall->getExprLoc(); + SourceRange ReplacementRange = SourceRange(ReplacementLoc, ReplacementLoc); + + CXXRecordDecl::method_range Methods = + MemberCall->getRecordDecl()->methods(); + DeclContext::specific_decl_iterator Clear = + llvm::find_if(Methods, [](const CXXMethodDecl *F) { + return F->getDeclName().getAsString() == "clear" && + F->getMinRequiredArguments() == 0; + }); + + if (Clear != Methods.end()) { + diag(MemberLoc, + "ignoring the result of 'empty()', did you mean 'clear()'? ") + << FixItHint::CreateReplacement(ReplacementRange, "clear"); + } else { + diag(MemberLoc, "ignoring the result of 'empty()'"); + } + + } else if (const auto *NonMemberCall = + Result.Nodes.getNodeAs("empty")) { + SourceLocation NonMemberLoc = NonMemberCall->getExprLoc(); + SourceLocation NonMemberEndLoc = NonMemberCall->getEndLoc(); + + const Expr *Arg = NonMemberCall->getArg(0); + + CXXRecordDecl *ArgRecordDecl = Arg->getType()->getAsCXXRecordDecl(); + if (ArgRecordDecl == NULL) + return; + CXXRecordDecl::method_range Methods = ArgRecordDecl->methods(); + DeclContext::specific_decl_iterator Clear = + llvm::find_if(Methods, [](const CXXMethodDecl *F) { + return F->getDeclName().getAsString() == "clear" && + F->getMinRequiredArguments() == 0; + }); + + if (Clear != Methods.end()) { + std::string ReplacementText = + std::string(Lexer::getSourceText( + CharSourceRange::getTokenRange(Arg->getSourceRange()), + *Result.SourceManager, getLangOpts())) + + ".clear()"; + SourceRange ReplacementRange = SourceRange(NonMemberLoc, NonMemberEndLoc); + diag(NonMemberLoc, "ignoring the result of '%0', did you mean 'clear()'?") + << llvm::dyn_cast(NonMemberCall->getCalleeDecl()) + ->getQualifiedNameAsString() + << FixItHint::CreateReplacement(ReplacementRange, ReplacementText); + } else { + diag(NonMemberLoc, "ignoring the result of '%0'") + << llvm::dyn_cast(NonMemberCall->getCalleeDecl()) + ->getQualifiedNameAsString(); + } + } +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -122,6 +122,11 @@ Finds initializations of C++ shared pointers to non-array type that are initialized with an array. +- New :doc:`bugprone-standalone-empty ` check. + + Warns when `empty()` is used on a range and the result is ignored. Suggests `clear()` as an alternative + if it is an existing member function. + - New :doc:`bugprone-unchecked-optional-access ` check. 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst @@ -0,0 +1,30 @@ +.. title:: clang-tidy - bugprone-standalone-empty + +bugprone-standalone-empty +========================= + +Checks for inaccurate use of the ``empty()`` method. + +The ``empty()`` method on several common ranges returns a Boolean indicating +whether or not the range is empty, but is often mistakenly interpreted as +a way to clear the contents of a range. Some ranges offer a ``clear()`` +method for this purpose. This check warns when a call to empty returns a +result that is ignored, and suggests replacing it with a call to ``clear()`` +if it is available as a member function of the range. + +For example, the following code could be used to indicate whether a range +is empty or not, but the result is ignored: + +.. code-block:: c++ + + std::vector v; + ... + v.empty(); + +A call to ``clear()`` would appropriately clear the contents of the range: + +.. code-block:: c++ + + std::vector v; + ... + v.clear(); diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -114,6 +114,7 @@ `bugprone-sizeof-container `_, `bugprone-sizeof-expression `_, `bugprone-spuriously-wake-up-functions `_, + `bugprone-standalone-empty `_, "Yes" `bugprone-string-constructor `_, "Yes" `bugprone-string-integer-assignment `_, "Yes" `bugprone-string-literal-with-embedded-nul `_, 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp @@ -0,0 +1,244 @@ +// RUN: %check_clang_tidy %s bugprone-standalone-empty %t + +namespace std { +template +struct vector { + bool empty(); +}; + +template +struct vector_with_clear { + bool empty(); + void clear(); +}; + +template +struct vector_with_void_empty { + void empty(); + void clear(); +}; + +template +struct vector_with_int_empty { + int empty(); + void clear(); +}; + +template +bool empty(T &&); + +} // namespace std + +namespace absl { +struct string { + bool empty(); +}; + +struct string_with_clear { + bool empty(); + void clear(); +}; + +struct string_with_void_empty { + void empty(); + void clear(); +}; + +struct string_with_int_empty { + int empty(); + void clear(); +}; + +template +bool empty(T &&); +} // namespace absl + +namespace test { +template +void empty(T &&); +} // namespace test + +int test_member_empty() { + { + std::vector v; + + v.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + + bool test = v.empty(); + //(void) v.empty(); + // no-warning + // NOTE: This test not currently passing. + } + + { + std::vector_with_void_empty v; + + v.empty(); + // no-warning + } + + { + std::vector_with_clear 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();{{$}} + } + + { + std::vector_with_int_empty 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();{{$}} + } + + { + absl::string s; + + s.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()' [bugprone-standalone-empty] + } + + { + absl::string_with_void_empty s; + + s.empty(); + // no-warning + } + + { + absl::string_with_clear s; + + s.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } + + { + absl::string_with_int_empty s; + + s.empty(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'empty()', did you mean 'clear()'? [bugprone-standalone-empty] + // CHECK-FIXES: {{^ }} s.clear();{{$}} + } +} + +int 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] + // CHECK-FIXES: {{^ }} v.clear();{{$}} + + test::empty(v); + // no-warning + } + + { + absl::string s; + + std::empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } +} + +int test_unqualified_empty() { + { + std::vector v; + + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } + + { + std::vector_with_void_empty v; + + 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();{{$}} + } + + { + std::vector_with_clear v; + + 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();{{$}} + } + + { + std::vector_with_int_empty v; + + 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::string s; + + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty' [bugprone-standalone-empty] + } + + { + absl::string_with_void_empty s; + + 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();{{$}} + } + + { + absl::string_with_clear s; + + 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();{{$}} + } + + { + absl::string_with_int_empty s; + + 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 v; + using std::empty; + empty(v); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'std::empty' [bugprone-standalone-empty] + } + + { + std::vector_with_clear v; + using std::empty; + 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::string s; + using absl::empty; + empty(s); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: ignoring the result of 'absl::empty' [bugprone-standalone-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();{{$}} + } +}