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,51 @@ +//===--- 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; + bool ignoreInternalFunctions; +}; + +} // 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,131 @@ +//===--- 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(QualType ParamType) { + return ParamType->isReferenceType() && + !ParamType.getNonReferenceType().isConstQualified(); +} + +static bool doesFunctionNameStartWithUnderScore(const FunctionDecl *D) { + return D->getNameAsString().find("_") == 0; +} + +namespace { +AST_MATCHER(CXXMethodDecl, isOverloadedOperator) { + // don't put [[nodiscard]] 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_P(CXXMethodDecl, isInternalFunction, bool, ignore) { + // don't add [[nodiscard]] to anything marked as an internal + // function like _Foo() + if (ignore){ + return doesFunctionNameStartWithUnderScore(&Node); + } + return false; +} +AST_MATCHER(CXXMethodDecl, hasNonConstReferenceOrPointerArguments) { + // if the function has any non constant reference parameters + // e.g. foo(A &a) + // or pointer arguments + // foo(A*) + // then they may not care about the return because we may be passing data + // via the arguments however + // functions with no arguments will fall through foo() + // + for (const auto *Par : Node.parameters()) { + const Type *ParType = Par->getType().getTypePtr(); + + if (isNonConstReferenceType(Par->getType())) { + return true; + } + if (ParType->isPointerType()) { + return true; + } + } + return false; + } +} // namespace + +UseNodiscardCheck::UseNodiscardCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + NoDiscardMacro(Options.get("ReplacementString", "[[nodiscard]]")), + ignoreInternalFunctions(Options.get("IgnoreInternalFunctions", true)) {} + +void UseNodiscardCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "ReplacementString", NoDiscardMacro); + Options.store(Opts, "IgnoreInternalFunctions", ignoreInternalFunctions); +} + +void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) { + // if we are using C++17 attributes we are going to need c++17 + if (NoDiscardMacro == "[[nodiscard]]") { + if (!getLangOpts().CPlusPlus17) + return; + } + + // if we are using a macro we can simply set it to blank when + // not using c++17 compilers + if (!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(isInternalFunction(ignoreInternalFunctions)), + unless(hasNonConstReferenceOrPointerArguments()), + unless(hasAttr(clang::attr::WarnUnusedResult)))) + .bind("no_discard"), + this); +} + +void UseNodiscardCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("no_discard"); + + // if the location is invalid forget it + if (!MatchedDecl->getLocation().isValid()) + 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 return a variable which is ignored + // performs some externation io operation and the return value could be ignore + + SourceLocation retLoc = MatchedDecl->getInnerLocStart(); + + // This member function could be marked [[nodiscard]] + diag(retLoc, "function %0 should be marked " + NoDiscardMacro) + << MatchedDecl + << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " "); + return; +} + +} // namespace modernize +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -167,6 +167,13 @@ 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 where the return value of a function + 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 @@ -12,8 +12,8 @@ abseil-no-internal-dependencies abseil-no-namespace abseil-redundant-strcat-calls - abseil-string-find-startswith abseil-str-cat-append + abseil-string-find-startswith android-cloexec-accept android-cloexec-accept4 android-cloexec-creat @@ -89,8 +89,8 @@ cert-msc50-cpp cert-msc51-cpp cert-oop11-cpp (redirects to performance-move-constructor-init) - cppcoreguidelines-avoid-goto cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) + cppcoreguidelines-avoid-goto cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) cppcoreguidelines-interfaces-global-init @@ -157,6 +157,7 @@ hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) hicpp-static-assert (redirects to misc-static-assert) hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) + hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) hicpp-use-auto (redirects to modernize-use-auto) hicpp-use-emplace (redirects to modernize-use-emplace) hicpp-use-equals-default (redirects to modernize-use-equals-default) @@ -165,7 +166,6 @@ hicpp-use-nullptr (redirects to modernize-use-nullptr) hicpp-use-override (redirects to modernize-use-override) hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) - hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) llvm-header-guard llvm-include-order llvm-namespace-comment @@ -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,78 @@ +.. title:: clang-tidy - modernize-use-nodiscard + +modernize-use-nodiscard +======================= + +Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member +functions to highlight at compile time where the return value of a function +should not be ignored. + +Member functions which return non void types, and take in only pass by value +or non constant references, and non pointer arguments, and of which the +member functions are themselve const, have not means of altering any state +or passing values other than the return type, unless they are using mutable +member variables or altering via some external call (e.g. IO). + +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 + +Users can use :option:`ReplacementString` to specify a macro to use instead of +``[[nodiscard]]``. This is useful when maintaining source code that needs to +also compile with a non c++17 conforming 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`. + +.. option:: IgnoreInternalFunctions + +Users can use :option:`IgnoreInternalFunctions` to turn off the adding of +``[nodiscard]]`` to functions starting with _ e.g. _Foo() + +Example +^^^^^^^ + +.. code-block:: c++ + + bool _Foo() const; + +transforms to: + + +.. code-block:: c++ + + bool _Fot() const; + +if the :option:`IgnoreInternalFunctions` option is set to `false`. Index: test/clang-tidy/modernize-use-nodiscard.cpp =================================================================== --- /dev/null +++ test/clang-tidy/modernize-use-nodiscard.cpp @@ -0,0 +1,138 @@ +// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \ +// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \ +// RUN: -- -std=c++17 \ + +// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]] +#define MUST_USE_RESULT __attribute__((warn_unused_result)) +#define NO_DISCARD [[nodiscard]] +#define NO_RETURN [[noreturn]] + +class Foo +{ +public: + 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: + // CHECK-FIXES: void f5() const; + + bool f6(); + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool f6(); + + bool f7(int &); + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool f7(int &); + + bool f8(int &) const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool f8(int &) const; + + bool f9(int *) const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool f9(int *) const; + + bool f10(const int &,int &) const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool f10(const int &,int &) const; + + NO_DISCARD bool f12() const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: NO_DISCARD bool f12() const; + + MUST_USE_RESULT bool f13() const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: MUST_USE_RESULT bool f13() const; + + // TODO fix CHECK-FIXES to handle "[[" "]]" + [[nodiscard]] bool f11() const; + // CHECK-MESSAGES-NOT: warning: + // _CHECK-FIXES: [[nodiscard]] bool f11() const; + + bool _f20() const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool _f20() const; + + NO_RETURN bool f21() const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: NO_RETURN bool f21() const; + + ~Foo(); + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: ~Foo(); + + bool operator +=(int) const; + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool operator +=(int) const; + + // 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; +}; + +bool Foo::f19() const + // CHECK-MESSAGES-NOT: warning: + // CHECK-FIXES: bool Foo::f19() const +{ + return true; +} + +template +class Bar +{ + public: + 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; +}; + +template +bool Bar::empty() const +// CHECK-MESSAGES-NOT: warning: +// CHECK-FIXES: bool Bar::empty() const +{ + return true; +} + +