MyDeveloperDay updated this revision to Diff 177584.
MyDeveloperDay marked 11 inline comments as done.
MyDeveloperDay added a comment.
Addressing review comments
- grammatical errors in documentation and comments
- prevent adding [[nodiscard]] to macros
- clean up list.rst
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55433/new/
https://reviews.llvm.org/D55433
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseNodiscardCheck.cpp
clang-tidy/modernize/UseNodiscardCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/modernize-use-nodiscard.rst
test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
test/clang-tidy/modernize-use-nodiscard.cpp
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,142 @@
+// 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
+
+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;
+
+ [[nodiscard]] bool f11() const;
+ // CHECK-MESSAGES-NOT: warning:
+ // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+ 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:
+ // 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;
+
+ BOOLEAN_FUNC;
+ // CHECK-MESSAGES-NOT: warning:
+ // CHECK-FIXES: BOOLEAN_FUNC;
+};
+
+bool Foo::f19() const
+ // CHECK-MESSAGES-NOT: warning:
+ // CHECK-FIXES: bool Foo::f19() const
+{
+ return true;
+}
+
+template<class T>
+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<class T>
+bool Bar<T>::empty() const
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool Bar<T>::empty() const
+{
+ return true;
+}
+
+
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-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: docs/clang-tidy/checks/modernize-use-nodiscard.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-nodiscard.rst
@@ -0,0 +1,62 @@
+.. title:: clang-tidy - modernize-use-nodiscard
+
+modernize-use-nodiscard
+=======================
+
+Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions to
+highlight at compile time which return values 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 themselves const, 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 altering 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: 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/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
+ <clang-tidy/checks/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
<clang-tidy/checks/readability-isolate-declaration>` check.
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,110 @@
+//===--- 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]] 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 (isNonConstReferenceType(ParType))
+ return true;
+ if (ParType->isPointerType())
+ 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 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(hasNonConstReferenceOrPointerArguments()),
+ unless(hasAttr(clang::attr::WarnUnusedResult))))
+ .bind("no_discard"),
+ this);
+}
+
+void UseNodiscardCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXMethodDecl>("no_discard");
+ // Don't make replacements if the location is invalid or in a macro.
+ SourceLocation Loc = MatchedDecl->getLocation();
+ if (!Loc.isValid() || 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 + " ");
+ return;
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
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<UseEqualsDefaultCheck>("modernize-use-equals-default");
CheckFactories.registerCheck<UseEqualsDeleteCheck>(
"modernize-use-equals-delete");
+ CheckFactories.registerCheck<UseNodiscardCheck>(
+ "modernize-use-nodiscard");
CheckFactories.registerCheck<UseNoexceptCheck>("modernize-use-noexcept");
CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
