Index: clang-tidy/modernize/CMakeLists.txt =================================================================== --- clang-tidy/modernize/CMakeLists.txt +++ clang-tidy/modernize/CMakeLists.txt @@ -26,6 +26,7 @@ UseEmplaceCheck.cpp UseEqualsDefaultCheck.cpp UseEqualsDeleteCheck.cpp + UseNodiscardCheck.cpp UseNoexceptCheck.cpp UseNullptrCheck.cpp UseOverrideCheck.cpp Index: clang-tidy/modernize/ModernizeTidyModule.cpp =================================================================== --- clang-tidy/modernize/ModernizeTidyModule.cpp +++ clang-tidy/modernize/ModernizeTidyModule.cpp @@ -32,6 +32,7 @@ #include "UseEmplaceCheck.h" #include "UseEqualsDefaultCheck.h" #include "UseEqualsDeleteCheck.h" +#include "UseNodiscardCheck.h" #include "UseNoexceptCheck.h" #include "UseNullptrCheck.h" #include "UseOverrideCheck.h" @@ -82,6 +83,8 @@ CheckFactories.registerCheck("modernize-use-equals-default"); CheckFactories.registerCheck( "modernize-use-equals-delete"); + CheckFactories.registerCheck( + "modernize-use-nodiscard"); CheckFactories.registerCheck("modernize-use-noexcept"); CheckFactories.registerCheck("modernize-use-nullptr"); CheckFactories.registerCheck("modernize-use-override"); Index: clang-tidy/modernize/UseNodiscardCheck.h =================================================================== --- /dev/null +++ clang-tidy/modernize/UseNodiscardCheck.h @@ -0,0 +1,50 @@ +//===--- UseNodiscardCheck.h - clang-tidy -----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USENODISCARDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USENODISCARDCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace modernize { + +/// \brief Add ``[[nodiscard]]`` to non-void const-member functions with no +/// arguments or pass-by-value or pass by const-reference arguments. +/// \code +/// bool empty() const; +/// bool empty(const Bar &) const; +/// bool empty(int bar) const; +/// \endcode +/// Is converted to: +/// \code +/// [[nodiscard]] bool empty() const; +/// [[nodiscard]] bool empty(const Bar &) const; +/// [[nodiscard]] bool empty(int bar) const; +/// \endcode +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nodiscard.html +class UseNodiscardCheck : public ClangTidyCheck { +public: + UseNodiscardCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const std::string NoDiscardMacro; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USENODISCARDCHECK_H Index: clang-tidy/modernize/UseNodiscardCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/modernize/UseNodiscardCheck.cpp @@ -0,0 +1,112 @@ +//===--- UseNodiscardCheck.cpp - clang-tidy -------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "UseNodiscardCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace modernize { + +static bool isNonConstReferenceType(const QualType &ParamType) { + return ParamType->isReferenceType() && + !ParamType.getNonReferenceType().isConstQualified(); +} + +namespace { +AST_MATCHER(CXXMethodDecl, isOverloadedOperator) { + // Don't put [[nodiscard]] int front of operators. + return Node.isOverloadedOperator(); +} +AST_MATCHER(CXXMethodDecl, isDefinitionOrInline) { + // A function definition, with optional inline but not the declaration. + return !(Node.isThisDeclarationADefinition() && Node.isOutOfLine()); +} +AST_MATCHER(CXXMethodDecl, hasNonConstReferenceOrPointerArguments) { + // If the function has any non-const-reference arguments + // bool foo(A &a) + // or pointer arguments + // bool foo(A*) + // then they may not care about the return value, because of passing data + // via the arguments however functions with no arguments will fall through + // bool foo() + // + return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) { + QualType ParType = Par->getType(); + + if (ParType->isTemplateTypeParmType() || ParType->isPointerType() || + isNonConstReferenceType(ParType)) + return true; + + // Find a better way of detecting const-reference-template type + // parameters via using alias not detected by ``isTemplateTypeParamType()`` + if (ParType.getCanonicalType().getAsString().find("type-parameter-") != + std::string::npos) + return true; + + return false; + }); +} +} // namespace + +UseNodiscardCheck::UseNodiscardCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + NoDiscardMacro(Options.get("ReplacementString", "[[nodiscard]]")) {} + +void UseNodiscardCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "ReplacementString", NoDiscardMacro); +} + +void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) { + // If we use ``[[nodiscard]]`` attribute, we require at least C++17. Use a + // macro or ``__attribute__`` with pre c++17 compilers by using + // ReplacementString option. + if ((NoDiscardMacro == "[[nodiscard]]" && !getLangOpts().CPlusPlus17) || + !getLangOpts().CPlusPlus) + return; + + // Find all non-void const methods which have not already been marked to + // warn on unused result. + Finder->addMatcher( + cxxMethodDecl(allOf(unless(returns(voidType())), isConst(), + unless(isNoReturn()), unless(isOverloadedOperator()), + isDefinitionOrInline(), + unless(hasNonConstReferenceOrPointerArguments()), + unless(hasAttr(clang::attr::WarnUnusedResult)))) + .bind("no_discard"), + this); +} // namespace modernize + +void UseNodiscardCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("no_discard"); + // Don't make replacements if the location is invalid or in a macro. + SourceLocation Loc = MatchedDecl->getLocation(); + if (Loc.isInvalid() || Loc.isMacroID()) + return; + + // Possible false positives include: + // 1. Classes containing mutable member variables modified by a const member + // function and the return type of said function is ignored. + // 2. A const member function which returns a variable which is ignored + // but performs some external I/O operation and the return value could be + // ignored. + SourceLocation retLoc = MatchedDecl->getInnerLocStart(); + diag(retLoc, "function %0 should be marked " + NoDiscardMacro) + << MatchedDecl + << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " "); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -167,6 +167,12 @@ Detects usage of the deprecated member types of ``std::ios_base`` and replaces those that have a non-deprecated equivalent. +- New :doc:`modernize-use-nodiscard + ` check. + + Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions + to highlight at compile time which return values should not be ignored. + - New :doc:`readability-isolate-decl ` check. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -205,6 +205,7 @@ modernize-use-emplace modernize-use-equals-default modernize-use-equals-delete + modernize-use-nodiscard modernize-use-noexcept modernize-use-nullptr modernize-use-override Index: docs/clang-tidy/checks/modernize-use-nodiscard.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/modernize-use-nodiscard.rst @@ -0,0 +1,66 @@ +.. title:: clang-tidy - modernize-use-nodiscard + +modernize-use-nodiscard +======================= + +Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions in +order to highlight at compile time which return values should not be ignored. + +Member functions need to satisfy the following conditions to be considered by +this check: + - no ``[[nodiscard]]``, ``[[noreturn]]``, ``__attribute__((warn_unused_result))``, ``[[clang::warn_unused_result]]`` nor ``[[gcc::warn_unused_result]]`` attribute, + - non-void return type, + - const member function, + - no non-const reference parameters, + - no pointer parameters, + - no template parameters, + +Such functions have no means of altering any state or passing values other than +via the return type. Unless the member functions are using mutable member +variables or alteringing state via some external call (e.g. I/O). + +Example +------- + +.. code-block:: c++ + + bool empty() const; + bool empty(int i) const; + +transforms to: + +.. code-block:: c++ + + [[nodiscard] bool empty() const; + [[nodiscard] bool empty(int i) const; + +Options +------- + +.. option:: ReplacementString + +Specifies a macro to use instead of ``[[nodiscard]]``. This is useful when +maintaining source code that needs to compile with a pre-C++17 compiler. + +Example +^^^^^^^ + +.. code-block:: c++ + + bool empty() const; + bool empty(int i) const; + +transforms to: + +.. code-block:: c++ + + NO_DISCARD bool empty() const; + NO_DISCARD bool empty(int i) const; + +if the :option:`ReplacementString` option is set to `NO_DISCARD`. + +.. note:: + + For alternative ``__attribute__`` syntax options to mark functions as + ``[[nodiscard]]`` in non-c++17 source code. + See https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result Index: test/clang-tidy/modernize-use-nodiscard-clang-unused.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-nodiscard-clang-unused.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: '[[clang::warn_unused_result]]'}]}" \ +// RUN: -- -std=c++11 \ + +class Foo +{ +public: + bool f1() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f1() const; + + bool f2(int) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f2(int) const; + + bool f3(const int &) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f3(const int &) const; + + bool f4(void) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f4(void) const; + +}; + Index: test/clang-tidy/modernize-use-nodiscard-cxx11.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-nodiscard-cxx11.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: '__attribute__((warn_unused_result))'}]}" \ +// RUN: -- -std=c++11 \ + +class Foo +{ +public: + bool f1() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard] + // CHECK-FIXES: __attribute__((warn_unused_result)) bool f1() const; + + bool f2(int) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard] + // CHECK-FIXES: __attribute__((warn_unused_result)) bool f2(int) const; + + bool f3(const int &) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard] + // CHECK-FIXES: __attribute__((warn_unused_result)) bool f3(const int &) const; + + bool f4(void) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard] + // CHECK-FIXES: __attribute__((warn_unused_result)) bool f4(void) const; + +}; + Index: test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: '[[gcc::warn_unused_result]]'}]}" \ +// RUN: -- -std=c++11 \ + +class Foo +{ +public: + bool f1() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f1() const; + + bool f2(int) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f2(int) const; + + bool f3(const int &) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f3(const int &) const; + + bool f4(void) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f4(void) const; + +}; + Index: test/clang-tidy/modernize-use-nodiscard-no-macro.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-nodiscard-no-macro.cpp @@ -0,0 +1,23 @@ +// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \ +// RUN: -- -std=c++17 \ + +class Foo +{ +public: + bool f1() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f1() const; + + bool f2(int) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f2(int) const; + + bool f3(const int &) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f3(const int &) const; + + bool f4(void) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard] + // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f4(void) const; + +}; Index: test/clang-tidy/modernize-use-nodiscard.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-nodiscard.cpp @@ -0,0 +1,180 @@ +// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \ +// RUN: -- -std=c++17 \ + +#define MUST_USE_RESULT __attribute__((warn_unused_result)) +#define NO_DISCARD [[nodiscard]] +#define NO_RETURN [[noreturn]] + +#define BOOLEAN_FUNC bool f23() const + +typedef unsigned my_unsigned; +typedef unsigned& my_unsigned_reference; +typedef const unsigned& my_unsigned_const_reference; + +class Foo +{ +public: + using size_type = unsigned; + + bool f1() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f1() const; + + bool f2(int) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f2(int) const; + + bool f3(const int &) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f3(const int &) const; + + bool f4(void) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f4(void) const; + + // negative tests + + void f5() const; + // CHECK-MESSAGES-NOT: warning: + + bool f6(); + // CHECK-MESSAGES-NOT: warning: + + bool f7(int &); + // CHECK-MESSAGES-NOT: warning: + + bool f8(int &) const; + // CHECK-MESSAGES-NOT: warning: + + bool f9(int *) const; + // CHECK-MESSAGES-NOT: warning: + + bool f10(const int &,int &) const; + // CHECK-MESSAGES-NOT: warning: + + NO_DISCARD bool f12() const; + // CHECK-MESSAGES-NOT: warning: + + MUST_USE_RESULT bool f13() const; + // CHECK-MESSAGES-NOT: warning: + + [[nodiscard]] bool f11() const; + // CHECK-MESSAGES-NOT: warning: + + bool _f20() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool _f20() const; + + NO_RETURN bool f21() const; + // CHECK-MESSAGES-NOT: warning: + + ~Foo(); + // CHECK-MESSAGES-NOT: warning: + + bool operator +=(int) const; + // CHECK-MESSAGES-NOT: warning: + + // extra keywords (virtual,inline,const) on return type + + virtual bool f14() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD virtual bool f14() const; + + const bool f15() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD const bool f15() const; + + inline const bool f16() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD inline const bool f16() const; + + inline virtual const bool f17() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const; + + // inline with body + bool f18() const { + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f18() const { + return true; + } + + bool f19() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f19() const; + + BOOLEAN_FUNC; + // CHECK-MESSAGES-NOT: warning: + + bool f24(size_type) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f24' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f24(size_type) const; + + bool f28(my_unsigned) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f28' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f28(my_unsigned) const; + + bool f29(my_unsigned_reference) const; + // CHECK-MESSAGES-NOT: warning: + + bool f30(my_unsigned_const_reference) const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f30' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool f30(my_unsigned_const_reference) const; +}; + +bool Foo::f19() const + // CHECK-MESSAGES-NOT: warning: +{ + return true; +} + +template +class Bar +{ + public: + using value_type = T; + using reference = value_type&; + using const_reference = const value_type&; + + bool empty() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'empty' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD bool empty() const; + + // we cannot assume that the template parameter isn't a pointer + bool f25(value_type) const; + // CHECK-MESSAGES-NOT: warning: + + bool f27(reference) const; + // CHECK-MESSAGES-NOT: warning: + + typename T::value_type f35() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f35' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD typename T::value_type f35() const + + T f34() const; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f34' should be marked NO_DISCARD [modernize-use-nodiscard] + // CHECK-FIXES: NO_DISCARD T f34() const + + bool f31(T) const; + // CHECK-MESSAGES-NOT: warning: + + bool f33(T&) const; + // CHECK-MESSAGES-NOT: warning: + + bool f26(const_reference) const; + // CHECK-MESSAGES-NOT: warning: + + bool f32(const T&) const; + // CHECK-MESSAGES-NOT: warning: +}; + +template +bool Bar::empty() const +// CHECK-MESSAGES-NOT: warning: +{ + return true; +} + + +