[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D55433#1357483 , @MyDeveloperDay 
wrote:

> In D55433#1351707 , @JonasToth wrote:
>
> > > I do not have commit rights. I'm not sure what constitutes someone who 
> > > can commit, but let me contribute a little more before taking that step,  
> > > I have another idea for a checker I'd like to try after this one, I just 
> > > wanted to get one under my belt first.
> >
> > See this: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
> >
> > I will commit for you then. More patches always welcome ;)
>
>
> @JonasToth, I notice when you commit you commit in 2 places, is this by hand 
> or automatic? or if this process is mandated or written down somewhere?
>
> rCTE350760 : [clang-tidy] Adding a new 
> modernize use nodiscard checker
>  rL350760 : [clang-tidy] Adding a new 
> modernize use nodiscard checker
>
> Do you have any guidance on how YOU commit (especially in clang-tools-extra) 
> that might be useful to others just starting out?


These are done automatically, probably because its all LLVM but split
into multiple repositories, this changes with the new monorepo.

I use `arc` and `git-svn`, see
https://www.llvm.org/docs/Phabricator.html and
https://www.llvm.org/docs/GettingStarted.html#developers-work-with-git-svn


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1351707 , @JonasToth wrote:

> > I do not have commit rights. I'm not sure what constitutes someone who can 
> > commit, but let me contribute a little more before taking that step,  I 
> > have another idea for a checker I'd like to try after this one, I just 
> > wanted to get one under my belt first.
>
> See this: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
>
> I will commit for you then. More patches always welcome ;)


@JonasToth, I notice when you commit you commit in 2 places, is this by hand or 
automatic? or if this process is mandated or written down somewhere?

rCTE350760 : [clang-tidy] Adding a new 
modernize use nodiscard checker
rL350760 : [clang-tidy] Adding a new 
modernize use nodiscard checker

Do you have any guidance on how YOU commit (especially in clang-tools-extra) 
that might be useful to others just starting out?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350760: [clang-tidy] Adding a new modernize use nodiscard 
checker (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55433?vs=180687=180909#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-nodiscard.rst
  
clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-clang-unused.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  
clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard.h

Index: clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tools-extra/trunk/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-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
+++ clang-tools-extra/trunk/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-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.cpp
@@ -0,0 +1,145 @@
+//===--- 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 doesNoDiscardMacroExist(ASTContext ,
+const llvm::StringRef ) {
+  // Don't check for the Macro existence if we are using an attribute
+  // either a C++17 standard attribute or pre C++17 syntax
+  if (MacroId.startswith("[[") || MacroId.startswith("__attribute__"))
+return true;
+
+  // Otherwise look up the macro name in the context to see if its defined.
+  return Context.Idents.get(MacroId).hasMacroDefinition();
+}
+
+namespace {
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // Don't put ``[[nodiscard]]`` in front of operators.
+  return Node.isOverloadedOperator();
+}
+AST_MATCHER(CXXMethodDecl, isConversionOperator) {
+  // Don't put ``[[nodiscard]]`` in front of a conversion decl
+  // like operator bool().
+  return isa(Node);
+}
+AST_MATCHER(CXXMethodDecl, hasClassMutableFields) {
+  // Don't put ``[[nodiscard]]`` on functions on classes with
+  // mutable member variables.
+  return Node.getParent()->hasMutableFields();
+}
+AST_MATCHER(ParmVarDecl, hasParameterPack) {
+  // Don't put ``[[nodiscard]]`` on functions with parameter pack arguments.
+  return Node.isParameterPack();
+}
+AST_MATCHER(CXXMethodDecl, 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> I do not have commit rights. I'm not sure what constitutes someone who can 
> commit, but let me contribute a little more before taking that step,  I have 
> another idea for a checker I'd like to try after this one, I just wanted to 
> get one under my belt first.

See this: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

I will commit for you then. More patches always welcome ;)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1350999 , @JonasToth wrote:

> LGTM!
>  You verified that your fixes, fix the issues in LLVM? But it looks good to 
> go.


They look good, you asked before...

> P.S. did you request commit rights already?

I do not have commit rights. I'm not sure what constitutes someone who can 
commit, but let me contribute a little more before taking that step,  I have 
another idea for a checker I'd like to try after this one, I just wanted to get 
one under my belt first.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

LGTM!
You verified that your fixes, fix the issues in LLVM? But it looks good to go.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



In D55433#1349709 , @JonasToth wrote:

> No problem, thats why we test on real projects first, because there is always 
> something hidden in C++ :)
>
> Is there a test for the lambdas?


test/clang-tidy/modernize-use-nodiscard.cpp line 158  (which is how it 
manifested inside LLVM) as a lambda assigned to an auto


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 180687.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Address review comments

- add more lambda tests
- add nested lambda test
- remove hasParent() call,  (seems isConversionOperator()) seems to detect the 
CXXMethodsDecls within a lambda that were causing [[nodiscard]] to be added 
before the []{... )


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,256 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17
+
+#include 
+
+namespace boost {
+template 
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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;
+
+bool f6();
+
+bool f7(int &);
+
+bool f8(int &) const;
+
+bool f9(int *) const;
+
+bool f10(const int &, int &) const;
+
+NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+
+[[nodiscard]] bool f11() const;
+
+[[clang::warn_unused_result]] bool f11a() const;
+
+[[gnu::warn_unused_result]] bool f11b() 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;
+
+~Foo();
+
+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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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 
+// 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.



In D55433#1349709 , @JonasToth wrote:

> No problem, thats why we test on real projects first, because there is always 
> something hidden in C++ :)
>
> Is there a test for the lambdas?


test/clang-tidy/modernize-use-nodiscard.cpp line 158  (which is how it 
manifested inside LLVM) as a lambda assigned to an auto




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:106
+isVariadic(), hasTemplateReturnType(),
+hasParent(cxxRecordDecl(isLambda())),
+hasClassMutableFields(),

JonasToth wrote:
> what happens for nested lambdas?
> `hasParent` should be avoided if possible, as the `clangd` folks are 
> currently implementing partial traversal to only analyze "the latest change". 
> If you can, please rewrite that without `hasParent`
I'll not deny I wasn't sure how to do this one, and totally stole the idea from 
another checker

fuchsia/TrailingReturnCheck.cpp:
hasParent(cxxRecordDecl(isLambda())

> hasParent(cxxRecordDecl(isLambda())),

Given that the matcher is on all CXXMethodDecl, I couldn't quite see how I 
could determine the CXXMethodDecl was in a lambda, without going to the parent  

{F7804868}

without it I'd get

```
const auto nonConstReferenceType = [] {
   return true;
 };

``` 

transformed to


```
const auto nonConstReferenceType = [[nodiscard]] [] {
  return true;
};

```

Which whilst probably true, made for some ugly reading..

not sure about nested lambdas...sounds even harder!









CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



In D55433#1349709 , @JonasToth wrote:

> No problem, thats why we test on real projects first, because there is always 
> something hidden in C++ :)
>
> Is there a test for the lambdas?


test/clang-tidy/modernize-use-nodiscard.cpp line 158  (which is how it 
manifested inside LLVM) as a lambda assigned to an auto


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

No problem, thats why we test on real projects first, because there is always 
something hidden in C++ :)

Is there a test for the lambdas?




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:39
+AST_MATCHER(CXXMethodDecl, isConversionDecl) {
+  // Don't put ``[[nodiscard]]`` in front of a conversion decl
+  // like operator bool().

I would prefer `isConversionOperator`. Its consistent with 
`isOverloadedOperator` and uses the right word (`operator bool` is an operator)



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:106
+isVariadic(), hasTemplateReturnType(),
+hasParent(cxxRecordDecl(isLambda())),
+hasClassMutableFields(),

what happens for nested lambdas?
`hasParent` should be avoided if possible, as the `clangd` folks are currently 
implementing partial traversal to only analyze "the latest change". If you can, 
please rewrite that without `hasParent`



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:141
+
+// Do not add ``[[nodiscard]]`` to paramaeter packs.
+template 

typo, paramaeter


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 180625.
MyDeveloperDay added a comment.

Sorry to add another revision but, rerunning no-discard checker on LLVM code 
base threw up other [[nodiscard]] placements which whilst they MAY be correct, 
I don't think we should handle in this first pass.

- conversion functions e.g. operator bool()
- lambdas
- add tests to validate both


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,250 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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;
+
+bool f6();
+
+bool f7(int &);
+
+bool f8(int &) const;
+
+bool f9(int *) const;
+
+bool f10(const int &, int &) const;
+
+NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+
+[[nodiscard]] bool f11() const;
+
+[[clang::warn_unused_result]] bool f11a() const;
+
+[[gnu::warn_unused_result]] bool f11b() 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;
+
+~Foo();
+
+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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D55433#1347553 , @MyDeveloperDay 
wrote:

> libprotobuf still builds cleanly with just the one expected warning..despite 
> adding over 500 [[nodiscards]]
>
> F7800873: image.png 
>
> I'll continue to look at other projects, but I'm happy at present that this 
> gives us a good starting point.


That looks good. If llvm (significant parts of it) are fine too, ready to go.

P.S. did you request commit rights already?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

libprotobuf still builds cleanly with just the one expected warning..despite 
adding over 500 [[nodiscards]]

F7800873: image.png 

I'll continue to look at other projects, but I'm happy at present that this 
gives us a good starting point.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

LGTM.

If you have the time please rerun this latest (final?) version over LLVM or any 
other bigger project and check if there are any issues left and ensure the code 
still compiles after code-transformation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:23
+static bool isAliasedTemplateParamType(QualType ParamType) {
+  return (ParamType.getCanonicalType()->isTemplateTypeParmType() ||
+  ParamType->isInstantiationDependentType());

JonasToth wrote:
> I think you don't need the first part of that condition. 
> `isInstantiationDependent` should include that, no?
> 
> I would prefer having this as a matcher, but i don't insist.
I agree, cleaner less code and easier to understand..moving it to a matcher



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:111
+ hasAnyParameter(anyOf(
+ parmVarDecl(hasType(references(functionObj))),
+ parmVarDecl(hasType(functionObj)),

JonasToth wrote:
> you can merge these two lines with `anyOf(functionObj, 
> references(functionObj))`, i think thats cleaner
I had to move the hasType inside the anyOf?



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:130
+  // Check for the existence of the keyword being used as the 
``[[nodiscard]]``.
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {
+diag(retLoc, "function %0 should be marked " + NoDiscardMacro)

JonasToth wrote:
> Please remove the duplication with `diag`.
> You can store the diagnostic in flight and append something afterwards:
> 
> ```
> auto Diag = diag(...);
> 
> if (canTransform)
>Diag << Change;
> ```
that feels cleaner


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 180266.
MyDeveloperDay marked 4 inline comments as done.
MyDeveloperDay added a comment.

Update with view comments from @JonasToth

- remove unnecessary static functions
- move checks into matchers
- remove duplicated diag call


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,218 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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;
+
+bool f6();
+
+bool f7(int &);
+
+bool f8(int &) const;
+
+bool f9(int *) const;
+
+bool f10(const int &, int &) const;
+
+NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+
+[[nodiscard]] bool f11() const;
+
+[[clang::warn_unused_result]] bool f11a() const;
+
+[[gnu::warn_unused_result]] bool f11b() 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;
+
+~Foo();
+
+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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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;
+}

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Mostly nits left. I think the check is good to go afterwards :)




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:23
+static bool isAliasedTemplateParamType(QualType ParamType) {
+  return (ParamType.getCanonicalType()->isTemplateTypeParmType() ||
+  ParamType->isInstantiationDependentType());

I think you don't need the first part of that condition. 
`isInstantiationDependent` should include that, no?

I would prefer having this as a matcher, but i don't insist.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:111
+ hasAnyParameter(anyOf(
+ parmVarDecl(hasType(references(functionObj))),
+ parmVarDecl(hasType(functionObj)),

you can merge these two lines with `anyOf(functionObj, 
references(functionObj))`, i think thats cleaner



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:128
+
+  auto  = *Result.Context;
+  // Check for the existence of the keyword being used as the 
``[[nodiscard]]``.

I would say ok-ish, but `ASTContext  = *Result.Context;` would be 
better.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:130
+  // Check for the existence of the keyword being used as the 
``[[nodiscard]]``.
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {
+diag(retLoc, "function %0 should be marked " + NoDiscardMacro)

Please remove the duplication with `diag`.
You can store the diagnostic in flight and append something afterwards:

```
auto Diag = diag(...);

if (canTransform)
   Diag << Change;
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:30
+// TODO: Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType ) {
+  // Try to catch both std::function and boost::function

JonasToth wrote:
> MyDeveloperDay wrote:
> > JonasToth wrote:
> > > Do you want a function-template (`template  void foo(T 
> > > argument);`) or the template-function `{std,boost}::function`?
> > > For the first the approach should be the same as above, for the second 
> > > your implementation might work.
> > > 
> > > But a matcher with `hasAnyName("::std::function", "::boost::function")` 
> > > would be cleaner if you can use that instead.
> > I can't seem to get the hasName or hasAnyName matcher pick up 
> > std::function<> as an argument
> > 
> > Using clang-query, I can't get this to work, did I miss something?
> > 
> > ```
> > match 
> > functionDecl(hasAnyParameter(hasType(namedDecl(hasName("::std::function").bind("x")
> > ```
> > 
> I experimented a bit and found something that might work for you. See the 
> attachment.
> {F7791969}
This really helped thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 180212.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Address review comments

- remove using strings to locate std::function and boost::function arguments 
using matcher with ``hasAnyName()`` instead
- add tests to check for const, const & for above


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,218 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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;
+
+bool f6();
+
+bool f7(int &);
+
+bool f8(int &) const;
+
+bool f9(int *) const;
+
+bool f10(const int &, int &) const;
+
+NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+
+[[nodiscard]] bool f11() const;
+
+[[clang::warn_unused_result]] bool f11a() const;
+
+[[gnu::warn_unused_result]] bool f11b() 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;
+
+~Foo();
+
+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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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: 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

let me look further into that, it works for std::function but not for const 
std::function or std::function &,  const std::function &


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:30
+// TODO: Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType ) {
+  // Try to catch both std::function and boost::function

MyDeveloperDay wrote:
> JonasToth wrote:
> > Do you want a function-template (`template  void foo(T 
> > argument);`) or the template-function `{std,boost}::function`?
> > For the first the approach should be the same as above, for the second your 
> > implementation might work.
> > 
> > But a matcher with `hasAnyName("::std::function", "::boost::function")` 
> > would be cleaner if you can use that instead.
> I can't seem to get the hasName or hasAnyName matcher pick up std::function<> 
> as an argument
> 
> Using clang-query, I can't get this to work, did I miss something?
> 
> ```
> match 
> functionDecl(hasAnyParameter(hasType(namedDecl(hasName("::std::function").bind("x")
> ```
> 
I experimented a bit and found something that might work for you. See the 
attachment.
{F7791969}


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:30
+// TODO: Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType ) {
+  // Try to catch both std::function and boost::function

JonasToth wrote:
> Do you want a function-template (`template  void foo(T argument);`) 
> or the template-function `{std,boost}::function`?
> For the first the approach should be the same as above, for the second your 
> implementation might work.
> 
> But a matcher with `hasAnyName("::std::function", "::boost::function")` would 
> be cleaner if you can use that instead.
I can't seem to get the hasName or hasAnyName matcher pick up std::function<> 
as an argument

Using clang-query, I can't get this to work, did I miss something?

```
match 
functionDecl(hasAnyParameter(hasType(namedDecl(hasName("::std::function").bind("x")
```



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 180073.
MyDeveloperDay marked 14 inline comments as done.
MyDeveloperDay added a comment.

Rebase

- update to address most review comments from @JonasToth
- simplify matcher (with less unless)
- remove the need to look for "type-parameter-" string


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,210 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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;
+
+bool f6();
+
+bool f7(int &);
+
+bool f8(int &) const;
+
+bool f9(int *) const;
+
+bool f10(const int &, int &) const;
+
+NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+
+[[nodiscard]] bool f11() const;
+
+[[clang::warn_unused_result]] bool f11a() const;
+
+[[gnu::warn_unused_result]] bool f11b() 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;
+
+~Foo();
+
+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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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 {
+

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:24
+// parameters via using alias not detected by ``isTemplateTypeParamType()``.
+static bool isAliasedTemplateParamType(const QualType ) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=

`QualType` is usually passed around as value, here and elsewhere.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType ) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+  std::string::npos);

Just in case you didn't read this: 
https://clang.llvm.org/docs/InternalsManual.html#the-type-class-and-its-subclasses
It helped me a lot understanding types in clang.

If you want to check if a type is a template-parameter 
`QT->isTemplateTypeParmType()` with `QualType QT;` should do it.

Explanation:

`operator->` is overloaded in `QualType` to go through the underlying `Type`. 
Rational is written in the manual.
As `QT` might be a typedef/alias these need to be resolved sometime -> 
`getCanonicalType()` which is a `QualType` itself.

Once you have such a `QualType` you should be able to query every aspect of it 
through the `operator->` interface. So if you want to check if the type is a 
reference: `QT->isReferenceType()`.

For the use-case it seems to me that `Type->isInstantiationDependentType()` or 
`Type->isDependentType()` are interesting?

Not sure If that already covers your exact need, but feel free to ask if you 
need more :)



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:30
+// TODO: Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType ) {
+  // Try to catch both std::function and boost::function

Do you want a function-template (`template  void foo(T argument);`) or 
the template-function `{std,boost}::function`?
For the first the approach should be the same as above, for the second your 
implementation might work.

But a matcher with `hasAnyName("::std::function", "::boost::function")` would 
be cleaner if you can use that instead.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32
+  // Try to catch both std::function and boost::function
+  return (ParamType.getAsString().find("::function") != std::string::npos);
+}

Do the parens suppress a warning or so? It is not consistently applied in all 
function, I would say eliding them is better.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:41
+static bool doesNoDiscardMacroExist(ASTContext ,
+const std::string ) {
+  // Don't check for the Macro existence if we are using an attribute

You could use a `llvm::StringRef` instead and use the more specific 
`startswith` instead.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:53
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // Don't put [[nodiscard]] in front of operators.
+  return Node.isOverloadedOperator();

please highlight `[[nodiscard]]` in these comments too.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:105
+  cxxMethodDecl(
+  allOf(
+  unless(returns(voidType())), isConst(), unless(isNoReturn()),

maybe demorgan simplification here? All the `unless()` seem redundant.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:114
+  this);
+} // namespace modernize
+

misplaced comment



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:126
+  auto  = *Result.Context;
+  // check for the existence of the keyword being used as the ``[[nodiscard]]``
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {

Nit: Please make that comment a full sentence (if it still exists after 
diag-change)



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:128
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {
+diag(retLoc, "function %0 could not be marked " + NoDiscardMacro +
+ " due to missing macro definition")

I would not emit different diagnostics, it complicates grepping and stuff.
Just in one case clang-tidy emits a fixit, in the other not.



Comment at: docs/ReleaseNotes.rst:26
 
-For more information about Clang or LLVM, including information about
-the latest release, please see the `Clang Web Site `_ 
or
-the `LLVM Web Site `_.
+For more information about Clang or LLVM, including information about the 
latest
+release, please see the `Clang Web Site `_ or the 

spurious change



Comment at: 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Sorry for my absence the last week, holidays came in between.
I will take a look at the code again and try to find a robuster way of 
template-type detection :) I do have an idea, but not sure if that works as 
expected (within this week).

Sorry again!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 179445.
MyDeveloperDay added a comment.

- Rebase
- Add a couple of extra test cases to ensure we don't add the NO_DISCARD macro 
when a clang or gcc attribute is present
- Ping to ask code owners to review and possibly commit if happy.


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,250 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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:
+
+[[clang::warn_unused_result]] bool f11a() const;
+// CHECK-MESSAGES-NOT: warning:
+
+[[gnu::warn_unused_result]] bool f11b() 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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178656.
MyDeveloperDay added a comment.

Remove unused matcher variable


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,244 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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 {
+// 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1334150 , @curdeius wrote:

> LGTM.
>  Any ideas for improvement, @JonasToth?
>  I'd rather have it merged and improve on it later if there are ideas on how 
> to do it better.


Thanks for the LGTM, as you know I don't have commit access, but I'm happy to 
wait for more people to review if there are concerns.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178654.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Fix review comments

- Fix minor spelling mistakes
- add additional boost::function unit test


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,244 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 
+
+#include 
+
+namespace boost {
+template  
+class function;
+}
+
+#include "modernize-use-nodiscard.h"
+
+#define BOOLEAN_FUNC bool f23() const
+
+typedef unsigned my_unsigned;
+typedef unsigned _unsigned_reference;
+typedef const unsigned _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 const std::string () const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f45' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const std::string () 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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

LGTM.
Any ideas for improvement, @JonasToth?
I'd rather have it merged and improve on it later if there are ideas on how to 
do it better.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32
+  // Try to catch both std::function and boost::function
+  return (ParamType.getAsString().find("::function") != std::string::npos);
+}

I see that you check for any `::function` but you only test `std::function`. 
Could you add a test case for `boost::function` or any other `xyz::function` 
please?



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:127
+  auto  = *Result.Context;
+  // check for the existance of the keyword being used as the ``[[nodiscard]]``
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro)) {

existance -> existence



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:231
+
+// don't mark typical ``[[nodisscard]]`` candidates if the class
+// has mutable member variables

Single 's' in `[[nodiscard]]`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178367.
MyDeveloperDay added a comment.

-Refactor the checker to remove the llvm::any_of by using a QualType 
ast-matcher with hasAnyParameter(hasType(x))

- i.e. ...unless(hasAnyParameter(hasType(isNonConstReferenceOrPointer(...

-Reduce the SNR ratio of using a macro as the ReplacementString option (e.g. 
LLVM_NODISCARD), when that macro is not define in-scope

- This was seen when performing clang-tidy on LLVM itself, some of the very low 
level classes did not have the LLVM_NODISCARD macro (from 
include/llvm/Support/Compiler.h) defined in scope.
- In these cases the fix-it caused too many additional changes to be needed of 
including the additional header file in order to build.
- Instead emit a warning to say that it could not be marked as LLVM_NODISCARD 
and don't apply the fix-it
- Add documentation and unit tests to document and validate the above


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp
  test/clang-tidy/modernize-use-nodiscard.h

Index: test/clang-tidy/modernize-use-nodiscard.h
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.h
@@ -0,0 +1,5 @@
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,244 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#include 
+
+#include "modernize-use-nodiscard.h"
+
+#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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:23
+Such functions have no means of altering any state or passing values other than
+via the return type. Unless the member functions are altering state via some 
external call (e.g. I/O).
+

noted > 80 char line will correct next patch


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178355.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

- Rebase
- Reduce false-positives by not adding [[nodiscard]] to functions if the class 
has mutable fields (removed as known issues) and add tests


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.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,238 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#include 
+
+#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: 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 7 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType ) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+  std::string::npos);

curdeius wrote:
> This indeed looks a bit ugly. Is there no check that skips const-ref template 
> params and handles `using`s / `typedef`s?
I'm half hoping @JonasToth might guide me to something better ;-) 



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:31
+static bool isFunctionTemplate(const QualType ) {
+  return (ParamType.getAsString().find("std::function") != std::string::npos);
+}

curdeius wrote:
> I'm not sure if you can find a better way to find parameters of type 
> `std::function` than this... Unless we find the rules that distinguish 
> function types from others.
> Why is `std::function` so different? How could we match `boost::function` and 
> alike types?
> 
> Just setting the questions, I have no answers.
> 
> Anyway, I think that this might be left for later to be improved.
I know, I didn't like it either... I was trying to exclude a lambda being 
passed in, just to limit the heuristic and thus reduce the SNR


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178217.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

Fix review comments

- minor changes to code comments
- add more unit tests for typedef'd template types


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.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,227 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#include 
+
+#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;
+  

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:22
+
+// Find a better way of detecting const-reference-template type
+// parameters via using alias not detected by ``isTemplateTypeParamType()``.

You can write `TODO: ` or `FIXME: ` in front of this comment to make it well 
visible.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:25
+static bool isAliasedTemplateParamType(const QualType ) {
+  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
+  std::string::npos);

This indeed looks a bit ugly. Is there no check that skips const-ref template 
params and handles `using`s / `typedef`s?



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+
+// Find a better way of detecting a function template.
+static bool isFunctionTemplate(const QualType ) {

`TODO: ` too.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:31
+static bool isFunctionTemplate(const QualType ) {
+  return (ParamType.getAsString().find("std::function") != std::string::npos);
+}

I'm not sure if you can find a better way to find parameters of type 
`std::function` than this... Unless we find the rules that distinguish function 
types from others.
Why is `std::function` so different? How could we match `boost::function` and 
alike types?

Just setting the questions, I have no answers.

Anyway, I think that this might be left for later to be improved.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+AST_MATCHER(CXXMethodDecl, hasTemplateReturnType) {
+  // Don't put [[nodiscard]] int front functions return T
+  return (Node.getReturnType()->isTemplateTypeParmType() ||

int front ... -> in front of functions that return T.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:23
+via the return type. Unless the member functions are using mutable member
+variables or alteringing state via some external call (e.g. I/O).
+

Typo: alteringing



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:162
+using reference = value_type&;
+using const_reference = const value_type&;
+

Could you use similar test cases for `typedef`s, please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

MyDeveloperDay wrote:
> MyDeveloperDay wrote:
> > JonasToth wrote:
> > > MyDeveloperDay wrote:
> > > > curdeius wrote:
> > > > > JonasToth wrote:
> > > > > > I think the template tests should be improved.
> > > > > > What happens for `T empty()`, `typename T::value_type empty()` and 
> > > > > > so on. THe same for the parameters for functions/methods (including 
> > > > > > function templates).
> > > > > > 
> > > > > > Thinking of it, it might be a good idea to ignore functions where 
> > > > > > the heuristic depends on the template-paramters.
> > > > > It might be a good idea to add a note in the documentation about 
> > > > > handling of function templates and functions in class templates.
> > > > I think I need some help to determine if the parameter is a template 
> > > > parameter (specially a const T & or a const_reference)
> > > > 
> > > > I'm trying to remove functions which have any type of Template 
> > > > parameter (at least for now)
> > > > 
> > > > I've modified the hasNonConstReferenceOrPointerArguments matcher to use 
> > > > isTemplateTypeParamType()
> > > > 
> > > > but this doesn't seem to work though an Alias or even just with a const 
> > > > &
> > > > 
> > > > ```
> > > >   return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
> > > > QualType ParType = Par->getType();
> > > > 
> > > > if (ParType->isTemplateTypeParmType())
> > > >   return true;
> > > > 
> > > > if (ParType->isPointerType() || isNonConstReferenceType(ParType))
> > > >   return true;
> > > > 
> > > > return false;
> > > >   });
> > > > ```
> > > > 
> > > > mostly the tests cases work for  T and T&  (see below)
> > > > 
> > > > but it does not seem to work for const T&, or const_reference, where it 
> > > > still wants to add the [[nodiscard]]
> > > > 
> > > > Could anyone give me any pointers, or somewhere I can look to learn?  I 
> > > > was thinking I needed to look at the getUnqualifiedDeSugared() but it 
> > > > didn't seem to work the way I expected.
> > > > 
> > > > ```
> > > > 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:
> > > > // CHECK-FIXES: bool f25(value_type) const;
> > > > 
> > > > bool f27(reference) const;
> > > > // CHECK-MESSAGES-NOT: warning:
> > > > // CHECK-FIXES: bool f27(reference) const
> > > > 
> > > > 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:
> > > > // CHECK-FIXES: bool f31(T) const
> > > > 
> > > > bool f33(T&) const;
> > > > // CHECK-MESSAGES-NOT: warning:
> > > > // CHECK-FIXES: bool f33(T&) const
> > > > 
> > > > // -  FIXME TESTS BELOW FAIL - //
> > > > bool f26(const_reference) const;
> > > > // CHECK-MESSAGES-NOT: warning:
> > > > // CHECK-FIXES: bool f26(const_reference) const;
> > > > 
> > > > bool f32(const T&) const;
> > > > // CHECK-MESSAGES-NOT: warning:
> > > > // CHECK-FIXES: bool f32(const T&) const
> > > > };
> > > > 
> > > > ```
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > Is this resolved, as you marked it done?
> > Still working on these last 2 cases, they don't seem to be excluded with 
> > the isTemplateTypeParmType() call
> > 
> > // -  FIXME TESTS BELOW FAIL - //
> > bool f26(const_reference) const;
> > // CHECK-MESSAGES-NOT: warning:
> > // CHECK-FIXES: bool f26(const_reference) const;
> > 
> > bool f32(const T&) const;
> > // CHECK-MESSAGES-NOT: warning:
> > // CHECK-FIXES: bool f32(const T&) const
> > 
> > Let me fix what I can and I'll send an updated revision
> template arguments are now ignored so these tests are fixed
I found detecting  `typename T::value_type` and  `const_reference`, 
particularly troublesome, in the end opting 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178123.
MyDeveloperDay added a comment.

Add additional constraints on member functions

- do not add [[nodiscard]] to variadic function
- do not add [[nodiscard]] to std::function templates argument functions
- do not add [[nodiscard]] to functions returning template types
- add unit test to ensure above


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.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,202 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#include 
+
+#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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

MyDeveloperDay wrote:
> JonasToth wrote:
> > MyDeveloperDay wrote:
> > > curdeius wrote:
> > > > JonasToth wrote:
> > > > > I think the template tests should be improved.
> > > > > What happens for `T empty()`, `typename T::value_type empty()` and so 
> > > > > on. THe same for the parameters for functions/methods (including 
> > > > > function templates).
> > > > > 
> > > > > Thinking of it, it might be a good idea to ignore functions where the 
> > > > > heuristic depends on the template-paramters.
> > > > It might be a good idea to add a note in the documentation about 
> > > > handling of function templates and functions in class templates.
> > > I think I need some help to determine if the parameter is a template 
> > > parameter (specially a const T & or a const_reference)
> > > 
> > > I'm trying to remove functions which have any type of Template parameter 
> > > (at least for now)
> > > 
> > > I've modified the hasNonConstReferenceOrPointerArguments matcher to use 
> > > isTemplateTypeParamType()
> > > 
> > > but this doesn't seem to work though an Alias or even just with a const &
> > > 
> > > ```
> > >   return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
> > > QualType ParType = Par->getType();
> > > 
> > > if (ParType->isTemplateTypeParmType())
> > >   return true;
> > > 
> > > if (ParType->isPointerType() || isNonConstReferenceType(ParType))
> > >   return true;
> > > 
> > > return false;
> > >   });
> > > ```
> > > 
> > > mostly the tests cases work for  T and T&  (see below)
> > > 
> > > but it does not seem to work for const T&, or const_reference, where it 
> > > still wants to add the [[nodiscard]]
> > > 
> > > Could anyone give me any pointers, or somewhere I can look to learn?  I 
> > > was thinking I needed to look at the getUnqualifiedDeSugared() but it 
> > > didn't seem to work the way I expected.
> > > 
> > > ```
> > > 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:
> > > // CHECK-FIXES: bool f25(value_type) const;
> > > 
> > > bool f27(reference) const;
> > > // CHECK-MESSAGES-NOT: warning:
> > > // CHECK-FIXES: bool f27(reference) const
> > > 
> > > 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:
> > > // CHECK-FIXES: bool f31(T) const
> > > 
> > > bool f33(T&) const;
> > > // CHECK-MESSAGES-NOT: warning:
> > > // CHECK-FIXES: bool f33(T&) const
> > > 
> > > // -  FIXME TESTS BELOW FAIL - //
> > > bool f26(const_reference) const;
> > > // CHECK-MESSAGES-NOT: warning:
> > > // CHECK-FIXES: bool f26(const_reference) const;
> > > 
> > > bool f32(const T&) const;
> > > // CHECK-MESSAGES-NOT: warning:
> > > // CHECK-FIXES: bool f32(const T&) const
> > > };
> > > 
> > > ```
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > Is this resolved, as you marked it done?
> Still working on these last 2 cases, they don't seem to be excluded with the 
> isTemplateTypeParmType() call
> 
> // -  FIXME TESTS BELOW FAIL - //
> bool f26(const_reference) const;
> // CHECK-MESSAGES-NOT: warning:
> // CHECK-FIXES: bool f26(const_reference) const;
> 
> bool f32(const T&) const;
> // CHECK-MESSAGES-NOT: warning:
> // CHECK-FIXES: bool f32(const T&) const
> 
> Let me fix what I can and I'll send an updated revision
template arguments are now ignored so these tests are fixed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 178108.
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

Addressing review comments

- don't add [[nodiscard]] onto function taking template arguments
- add more unit tests to cover other discard possibilities 
[[clang::warn_unused_result]],[[gcc::warn_unused_result]]
- improve the documentation to help explain when the [[nodiscard]] will/will 
not be applied


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.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,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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 6 inline comments as done.
MyDeveloperDay added a comment.



>> 
>> 
>>   unsigned BlockOrCode = 0;
>>   llvm::ErrorOr Res = skipUntilRecordOrBlock(Stream, BlockOrCode);
>>   if (!Res)
>> Res.getError();
>>
> 
> AFAIK `llvm::Error` must be consumed because if it goes out of scope 
> unhandled it will `assert`. Not sure how to handle that.

Actually in this case its the getError() that the offender




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:73
+  // c++17 compilers.
+  if (!getLangOpts().CPlusPlus)
+return;

curdeius wrote:
> I'd move this if to the bottom of the function as it's the most general one 
> and fix the comment above: e.g. `// Ignore non-C++ code.`.
merged into one if as suggested by @JonasToth 



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

JonasToth wrote:
> MyDeveloperDay wrote:
> > curdeius wrote:
> > > JonasToth wrote:
> > > > I think the template tests should be improved.
> > > > What happens for `T empty()`, `typename T::value_type empty()` and so 
> > > > on. THe same for the parameters for functions/methods (including 
> > > > function templates).
> > > > 
> > > > Thinking of it, it might be a good idea to ignore functions where the 
> > > > heuristic depends on the template-paramters.
> > > It might be a good idea to add a note in the documentation about handling 
> > > of function templates and functions in class templates.
> > I think I need some help to determine if the parameter is a template 
> > parameter (specially a const T & or a const_reference)
> > 
> > I'm trying to remove functions which have any type of Template parameter 
> > (at least for now)
> > 
> > I've modified the hasNonConstReferenceOrPointerArguments matcher to use 
> > isTemplateTypeParamType()
> > 
> > but this doesn't seem to work though an Alias or even just with a const &
> > 
> > ```
> >   return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
> > QualType ParType = Par->getType();
> > 
> > if (ParType->isTemplateTypeParmType())
> >   return true;
> > 
> > if (ParType->isPointerType() || isNonConstReferenceType(ParType))
> >   return true;
> > 
> > return false;
> >   });
> > ```
> > 
> > mostly the tests cases work for  T and T&  (see below)
> > 
> > but it does not seem to work for const T&, or const_reference, where it 
> > still wants to add the [[nodiscard]]
> > 
> > Could anyone give me any pointers, or somewhere I can look to learn?  I was 
> > thinking I needed to look at the getUnqualifiedDeSugared() but it didn't 
> > seem to work the way I expected.
> > 
> > ```
> > 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:
> > // CHECK-FIXES: bool f25(value_type) const;
> > 
> > bool f27(reference) const;
> > // CHECK-MESSAGES-NOT: warning:
> > // CHECK-FIXES: bool f27(reference) const
> > 
> > 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:
> > // CHECK-FIXES: bool f31(T) const
> > 
> > bool f33(T&) const;
> > // CHECK-MESSAGES-NOT: warning:
> > // CHECK-FIXES: bool f33(T&) const
> > 
> > // -  FIXME TESTS BELOW FAIL - //
> > bool f26(const_reference) const;
> > // CHECK-MESSAGES-NOT: warning:
> > // CHECK-FIXES: bool f26(const_reference) const;
> > 
> > bool f32(const T&) const;
> > // CHECK-MESSAGES-NOT: warning:
> > // CHECK-FIXES: bool f32(const T&) const
> > };
> > 
> > ```
> > 
> > 
> > 
> > 
> > 
> > 
> Is this resolved, as you marked it done?
Still working on these last 2 cases, they don't seem to be excluded with the 
isTemplateTypeParmType() call

// -  FIXME TESTS BELOW FAIL - //
bool f26(const_reference) const;
// CHECK-MESSAGES-NOT: warning:
// CHECK-FIXES: bool f26(const_reference) const;

bool f32(const T&) const;
// CHECK-MESSAGES-NOT: warning:
// CHECK-FIXES: bool f32(const T&) const

Let me fix what I can and I'll send an updated 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:183
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f33(T&) const
+

No warning -> No fix -> You can ellide the `CHECK-FIXES` here and elsewhere. 
FileCheck is not confused by that :)
You don't need to specify that you dont expect a warning, too, because every 
warning that is not handled by `CHECK-MESSAGES`/`CHECK-NOTES` will result in a 
failed test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

MyDeveloperDay wrote:
> curdeius wrote:
> > JonasToth wrote:
> > > I think the template tests should be improved.
> > > What happens for `T empty()`, `typename T::value_type empty()` and so on. 
> > > THe same for the parameters for functions/methods (including function 
> > > templates).
> > > 
> > > Thinking of it, it might be a good idea to ignore functions where the 
> > > heuristic depends on the template-paramters.
> > It might be a good idea to add a note in the documentation about handling 
> > of function templates and functions in class templates.
> I think I need some help to determine if the parameter is a template 
> parameter (specially a const T & or a const_reference)
> 
> I'm trying to remove functions which have any type of Template parameter (at 
> least for now)
> 
> I've modified the hasNonConstReferenceOrPointerArguments matcher to use 
> isTemplateTypeParamType()
> 
> but this doesn't seem to work though an Alias or even just with a const &
> 
> ```
>   return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
> QualType ParType = Par->getType();
> 
> if (ParType->isTemplateTypeParmType())
>   return true;
> 
> if (ParType->isPointerType() || isNonConstReferenceType(ParType))
>   return true;
> 
> return false;
>   });
> ```
> 
> mostly the tests cases work for  T and T&  (see below)
> 
> but it does not seem to work for const T&, or const_reference, where it still 
> wants to add the [[nodiscard]]
> 
> Could anyone give me any pointers, or somewhere I can look to learn?  I was 
> thinking I needed to look at the getUnqualifiedDeSugared() but it didn't seem 
> to work the way I expected.
> 
> ```
> 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:
> // CHECK-FIXES: bool f25(value_type) const;
> 
> bool f27(reference) const;
> // CHECK-MESSAGES-NOT: warning:
> // CHECK-FIXES: bool f27(reference) const
> 
> 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:
> // CHECK-FIXES: bool f31(T) const
> 
> bool f33(T&) const;
> // CHECK-MESSAGES-NOT: warning:
> // CHECK-FIXES: bool f33(T&) const
> 
> // -  FIXME TESTS BELOW FAIL - //
> bool f26(const_reference) const;
> // CHECK-MESSAGES-NOT: warning:
> // CHECK-FIXES: bool f26(const_reference) const;
> 
> bool f32(const T&) const;
> // CHECK-MESSAGES-NOT: warning:
> // CHECK-FIXES: bool f32(const T&) const
> };
> 
> ```
> 
> 
> 
> 
> 
> 
Is this resolved, as you marked it done?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> You also get into trouble because there are many header files that it add 
> LLVM_NODISCARD to but they don't include "Compiler.h" where LLVM is defined 
> (so you end up with lots of errors) 
>  3>C:\llvm\include\llvm/ADT/iterator_range.h(46): error C3646: 'IteratorT': 
> unknown override specifier (compiling source file 
> C:\llvm\tools\clang\utils\TableGen\ClangCommentHTMLNamedCharacterReferenceEmitter.cpp)
>  I'm wondering if there is anything I can add to the checker, to check to see 
> if the macro being used for the ReplacementString is defined "in scope"

There is `clang-tidy/utils/IncludeInserter.cpp` that helps you ensuring a 
necessary include is made. I am not sure if checking "in scope" is possible for 
macros in clang-tidy is practical, as it works on the AST that does not contain 
the macros. You can hook into `PPCallbacks` and check your macro exists and is 
defined. But will it be stable, as you can pass in `__attribute__(...)` which 
would not be a macro.
Maybe `IncludeInserter.cpp` is the best for now.

> Of course as I'm not building with C++17 I could have used [[nodiscard]] 
> instead of LLVM_NODISCARD, and that would of improved this I guess
>  I do get 100's of nodiscard warnings
>  the majority come from Diag() calls e.g.
> 
>   Diag(clang::diag::err_drv_expecting_fopenmp_with_fopenmp_targets);
>   
>   96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(591): warning C4834: 
> discarding return value of function with 'nodiscard' attribute
>   96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(688): warning C4834: 
> discarding return value of function with 'nodiscard' attribute
>   96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(749): warning C4834: 
> discarding return value of function with 'nodiscard' attribute
>   96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(795): warning C4834: 
> discarding return value of function with 'nodiscard' attribute

A heuristic could be, if the destructor is "trivial". Some classes do their 
business logic in the destructor (emitting logs, registering the diagnostic, 
...).
This pattern might be quite common for such tasks. Not annotating constructors 
might work, too. They are "special functions" in the language sense
and if the user calls the constructor and discards its result it probably means 
the destructor does something interesting.
Not sure if that is worth a heuristic...

> But I do get some other ones which look interesting, (I don't know these 
> areas of the code well enough to know if these are real issues!)
> 
> 90>C:\llvm\tools\clang\lib\Frontend\DiagnosticRenderer.cpp(476): warning 
> C4834: discarding return value of function with 'nodiscard' attribute
> 
>   static bool checkRangeForMacroArgExpansion(CharSourceRange Range,
>  const SourceManager ,
>  SourceLocation ArgumentLoc) {
> SourceLocation BegLoc = Range.getBegin(), EndLoc = Range.getEnd();
> while (BegLoc != EndLoc) {
>   if (!checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc))
> return false;
>   BegLoc.getLocWithOffset(1);
> }
>   
> return checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc);
>   }

Thats the offender, is it?

>   BegLoc.getLocWithOffset(1);

Looks like a bug to me, `getLocWithOffset` does not modify the sourcelocation 
itself. Maybe the `checkLocForMacroArgExpansion` modifies it? But otherwise 
that looks like a potential infinite loop.

> also a couple like this
> 
> 90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(133): 
> warning C4834: discarding return value of function with 'nodiscard' attribute
>  90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(175): 
> warning C4834: discarding return value of function with 'nodiscard' attribute
> 
>   unsigned BlockOrCode = 0;
>   llvm::ErrorOr Res = skipUntilRecordOrBlock(Stream, BlockOrCode);
>   if (!Res)
> Res.getError();
>

AFAIK `llvm::Error` must be consumed because if it goes out of scope unhandled 
it will `assert`. Not sure how to handle that.

> I could see that this level of noise might put people off, although this is 
> alot higher level of noise than I saw in both protobuf or in opencv which I 
> tried.
> 
> I wonder if it would be enough to put in some kind of exclusion regex list?
> 
> Reruning the build after having removed the LLVM_NODISCARD from Diag(...) to 
> see how much it quietens down
> 
> I'll continue looking to see if I find anything interesting.

Thanks for the evaluation. clang-tidy already has some quiet chatty checks and 
I think we should try to give reasonable SNR.
If we find some kind of heuristic we can apply we should do it, otherwise we 
could land it anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I wanted to leave a comment regarding running the [[nodiscard]] checker over 
all of clang... as requested by @JonasToth, @lebedev.ri  and @Szelethus

I'm not 100% sure how to present the results, but let me pick out a few 
high/low lights...

My efforts are somewhat thwarted by the fact I build and develop on windows 
(its ok, I use vim and command line tools!), but I run clang-tidy inside visual 
studio

My first problem is that on windows build of clang LLVM_NODISCARD is #defined 
as nothing unless you tell CMAKE to use C++17  (-DCMAKE_CXX_STANDARD="17" )

Then if you try and make llvm with VS2017 with C++17 turned on, you get into 
all sort of trouble,

You also get into trouble because there are many header files that it add 
LLVM_NODISCARD to but they don't include "Compiler.h" where LLVM is defined (so 
you end up with lots of errors)

3>C:\llvm\include\llvm/ADT/iterator_range.h(46): error C3646: 'IteratorT': 
unknown override specifier (compiling source file 
C:\llvm\tools\clang\utils\TableGen\ClangCommentHTMLNamedCharacterReferenceEmitter.cpp)

I'm wondering if there is anything I can add to the checker, to check to see if 
the macro being used for the ReplacementString is defined "in scope"

Of course as I'm not building with C++17 I could have used [[nodiscard]] 
instead of LLVM_NODISCARD, and that would of improved this I guess

I do get 100's of nodiscard warnings

the majority come from Diag() calls e.g.

  Diag(clang::diag::err_drv_expecting_fopenmp_with_fopenmp_targets);
  
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(591): warning C4834: discarding 
return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(688): warning C4834: discarding 
return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(749): warning C4834: discarding 
return value of function with 'nodiscard' attribute
  96>C:\llvm\tools\clang\lib\Driver\Driver.cpp(795): warning C4834: discarding 
return value of function with 'nodiscard' attribute

But I do get some other ones which look interesting, (I don't know these areas 
of the code well enough to know if these are real issues!)

90>C:\llvm\tools\clang\lib\Frontend\DiagnosticRenderer.cpp(476): warning C4834: 
discarding return value of function with 'nodiscard' attribute

  static bool checkRangeForMacroArgExpansion(CharSourceRange Range,
 const SourceManager ,
 SourceLocation ArgumentLoc) {
SourceLocation BegLoc = Range.getBegin(), EndLoc = Range.getEnd();
while (BegLoc != EndLoc) {
  if (!checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc))
return false;
  BegLoc.getLocWithOffset(1);
}
  
return checkLocForMacroArgExpansion(BegLoc, SM, ArgumentLoc);
  }

also a couple like this

90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(133): 
warning C4834: discarding return value of function with 'nodiscard' attribute
90>C:\llvm\tools\clang\lib\Frontend\SerializedDiagnosticReader.cpp(175): 
warning C4834: discarding return value of function with 'nodiscard' attribute

  unsigned BlockOrCode = 0;
  llvm::ErrorOr Res = skipUntilRecordOrBlock(Stream, BlockOrCode);
  if (!Res)
Res.getError();

I could see that this level of noise might put people off, although this is 
alot higher level of noise than I saw in both protobuf or in opencv which I 
tried.

I wonder if it would be enough to put in some kind of exclusion regex list?

Reruning the build after having removed the LLVM_NODISCARD from Diag(...) to 
see how much it quietens down

I'll continue looking to see if I find anything interesting.




Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

curdeius wrote:
> JonasToth wrote:
> > I think the template tests should be improved.
> > What happens for `T empty()`, `typename T::value_type empty()` and so on. 
> > THe same for the parameters for functions/methods (including function 
> > templates).
> > 
> > Thinking of it, it might be a good idea to ignore functions where the 
> > heuristic depends on the template-paramters.
> It might be a good idea to add a note in the documentation about handling of 
> function templates and functions in class templates.
I think I need some help to determine if the parameter is a template parameter 
(specially a const T & or a const_reference)

I'm trying to remove functions which have any type of Template parameter (at 
least for now)

I've modified the hasNonConstReferenceOrPointerArguments matcher to use 
isTemplateTypeParamType()

but this doesn't seem to work though an Alias or even just with a const &

```
  return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
QualType ParType = Par->getType();

if (ParType->isTemplateTypeParmType())
  return true;

if (ParType->isPointerType() || 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177679.
MyDeveloperDay marked 10 inline comments as done.
MyDeveloperDay added a comment.

Resolving some (not all yet) review comments, and looking for help on template 
parameter exclusion

- add additional template argument tests
- add additional clang-warn-unused-result tests
- add additional gcc-warn-unused-result tests
- exclude template parameters
- improve nodiscard wordage in documentation regarding conditions of checker


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-clang-unused.cpp
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-gcc-unused.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,202 @@
+// 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:
+// 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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:65
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  // If we are using C++17 attributes we are going to need c++17
+  if (NoDiscardMacro == "[[nodiscard]]") {

I'd suggets: ` // If we use [[nodiscard]] attribute, we require at least C++17.`



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:73
+  // c++17 compilers.
+  if (!getLangOpts().CPlusPlus)
+return;

I'd move this if to the bottom of the function as it's the most general one and 
fix the comment above: e.g. `// Ignore non-C++ code.`.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:92
+  SourceLocation Loc = MatchedDecl->getLocation();
+  if (!Loc.isValid() || Loc.isMacroID())
+return;

You can simplify `!Loc.isValid()` to `Loc.isInvalid()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

A few more ideas for enhancements.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:41
+  //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

Double space after comma. Please check all comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:9
+
+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

`non-void`, `pass-by-value`, `non-const`, `non-pointer` with hyphens.
You may simplify `, and of which ...` (if you have an idea how to do it) as 
it's a bit hard to read.

The phrase starting with `Unless` is actually not a full phrase.

I'd suggest something along the lines:

```
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,
  - no non-const reference parameters,
  - no non-const pointer parameters,
...
```



Comment at: test/clang-tidy/modernize-use-nodiscard-cxx11.cpp:24
+
+};
+

I think that it would be cool to test that the check doesn't warn on 
`[[clang::warn_unused_result]]` nor `[[gcc::warn_unused_result]]`.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

JonasToth wrote:
> I think the template tests should be improved.
> What happens for `T empty()`, `typename T::value_type empty()` and so on. THe 
> same for the parameters for functions/methods (including function templates).
> 
> Thinking of it, it might be a good idea to ignore functions where the 
> heuristic depends on the template-paramters.
It might be a good idea to add a note in the documentation about handling of 
function templates and functions in class templates.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // Don't put [[nodiscard]] front of operators.
+  return Node.isOverloadedOperator();

s/front/in front/



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  return llvm::any_of(Node.parameters(), [](const ParmVarDecl *Par) {
+QualType  = Par->getType();
+

please remove the reference.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:66
+  // If we are using C++17 attributes we are going to need c++17
+  if (NoDiscardMacro == "[[nodiscard]]") {
+if (!getLangOpts().CPlusPlus17)

please merge the `if` conditions with `&&`, you could even add the next if with 
an `||`, but with proper parens.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:105
+  << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+  return;
+}

this `return` is not necessary



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:11
+
+class Foo
+{

Please add test that uses typedefs and `using` for the types that are passed 
into the functions.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:126
+template
+class Bar
+{

I think the template tests should be improved.
What happens for `T empty()`, `typename T::value_type empty()` and so on. THe 
same for the parameters for functions/methods (including function templates).

Thinking of it, it might be a good idea to ignore functions where the heuristic 
depends on the template-paramters.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:49
+
+
+.. code-block:: c++

Unnecessary empty line.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

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;
+}
+
+   

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Hi MyDeveloperDay,

thanks for the patch! Mostly stylistic comments. Would it make sense to attach 
the attribute to the implementation of the functions too?

This check is definitly useful, but noisy. Do you see a change of another 
heuristic that could be applied to reduce noise? Did you run the check over 
LLVM as this is our common experiment.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // don't put [[nodiscard]] front of operators
+  return Node.isOverloadedOperator();

Please make this comment a full sentence with punctuation and correct 
grammar/spelling. Same with other comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //bool foo()
+  return std::any_of(Node.param_begin(), Node.param_end(), [](const 
ParmVarDecl *Par) {
+const QualType  = Par->getType();

you can take `llvm::any_of(Node.parameters(), ...)` as a range-based version of 
the algorithm.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:46
+  return std::any_of(Node.param_begin(), Node.param_end(), [](const 
ParmVarDecl *Par) {
+const QualType  = Par->getType();
+

`QualType` is usually used as value, because its small. Same above. Once its a 
value, please ellide the `const` as llvm does not apply it to values.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:48
+
+if (isNonConstReferenceType(ParType)) {
+  return true;

You can ellide the braces.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:95
+  // if the location is invalid forget it
+  if (!MatchedDecl->getLocation().isValid())
+return;

Please bail on `isMacroID()` as well, as touching macros can have unpleasant 
side effects and is usually avoided in clang-tidy.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:104
+  // ignored
+
+  SourceLocation retLoc = MatchedDecl->getInnerLocStart();

you can remove that empty line.



Comment at: clang-tidy/modernize/UseNodiscardCheck.h:19
+
+/// \brief Add [[nodiscard]] to non void const member functions with no
+/// arguments or pass by value or pass by const reference arguments

Please surround code-snippets in comments with quotation. As iam bad with 
english with a grain of salt: `non-void` and `const-member-functions`? I feel 
that there need to be hyphens somewhere.



Comment at: docs/ReleaseNotes.rst:174
+  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.

I feel `where` sounds a bit weird here. Maybe `which return values should not 
be ignored`?



Comment at: docs/clang-tidy/checks/list.rst:15
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append

please remove the spurious changes here.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:38
+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.
+

Can you please point the users to 
https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
 to show the `__attribute__` syntax that is supported for non-c++17.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:9
+
+class Foo
+{

Please add tests, were macros generate the function decls and ensure these are 
untouched.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@curdeius Thanks, I don't have commit access so I'm happy wait for a 
CODE_OWNER, they could likely have more input.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM. But I'm not a code owner here and I don't know if you need an acceptance 
of one of them.
Great job.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177532.
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

Addressing review comments,

- additional unit tests for no ReplacementString and C++ 11 case
- more expressive hasNonConstReferenceOrPointerArguments matcher
- minor documentation typos and spacing


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,136 @@
+// 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]]
+
+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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+const Type *ParType = Par->getType().getTypePtr();

curdeius wrote:
> Wouldn't something along the lines:
> ```
> const auto  = Node.parameters();
> return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto 
> *Par) {
>   const Type *ParType = Par->getType().getTypePtr();
> 
>   if (isNonConstReferenceType(Par->getType())) {
> return true;
>   }
>   if (ParType->isPointerType()) {
> return true;
>   }
>   return false;
> });
> ```
> be a little bit more expressive?
> 
> I would also refactor it differently:
> ```
>   const Type  = Par->getType(); // not sure about Type
> 
>   if (isNonConstReferenceType(ParType)) {
>   // ...
>   if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() 
> possible?
>   // ...
> ```
> but that's a matter of taste.
I've just seen that you can use `.param_begin()` and `.param_end()` instead of 
`parameters.begin()/end()`.
I'd also reduce the use of `auto` here, because it's pretty hard to read it IMO.
So, I'd suggest

```
return std::any_of(Node.param_begin(), Node.param_end(), [](const ParmVarDecl 
*Par) {
  const QualType  = Par->getType();

  if (isNonConstReferenceType(ParType)) {
return true;
  }
  if (ParType.getTypePtr()->isPointerType()) {
return true;
  }
  return false;
});
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Some more minor remarks. I'll give this check a try to see how it behaves on 
some of my projects.
I agree that a high rate of false positives is possible (and is a bit of 
spoiler) but I wouldn't reject this IMO useful check because of that.
Anyway, everything looks pretty clean for me.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+const Type *ParType = Par->getType().getTypePtr();

Wouldn't something along the lines:
```
const auto  = Node.parameters();
return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto *Par) {
  const Type *ParType = Par->getType().getTypePtr();

  if (isNonConstReferenceType(Par->getType())) {
return true;
  }
  if (ParType->isPointerType()) {
return true;
  }
  return false;
});
```
be a little bit more expressive?

I would also refactor it differently:
```
  const Type  = Par->getType(); // not sure about Type

  if (isNonConstReferenceType(ParType)) {
  // ...
  if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() 
possible?
  // ...
```
but that's a matter of taste.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:12
+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 

themselve -> themselves, have not means -> have no means



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:3
+// RUN:   -config="{CheckOptions: [{key: 
modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+

I'm not sure what guidelines dictate, but I'd love to see another test file 
with `-std=c++14` for instance (or whatever minimal version to necessary to use 
`clang::WarnUnusedResult`, if any).
A very small test would be ok, you can also move some parts of this file into a 
new one.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;

You might want to get rid of this `TODO` note now, same for the one on the top 
of the file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Specify a macro to use instead of ``[[nodiscard]]``.  This is useful when 
+maintaining source code that needs to compile with a pre-C++17 compiler.

Option specif**ies**. Please fix double space.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177496.
MyDeveloperDay added a comment.

Update the documentation to utilize 80 character lines


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.cpp

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: :[[@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: 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Specify a macro to use instead of ``[[nodiscard]]``. 
+This is useful when maintaining source code that needs to compile with a 
pre-C++17 compiler.

Specifies. Please wrap up to 80 characters.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1325117 , @lebedev.ri wrote:

> In D55433#1323779 , @lebedev.ri 
> wrote:
>
> > In D55433#1323757 , 
> > @MyDeveloperDay wrote:
> >
> > > a lot of projects aren't setup for c++17 yet which is needed for 
> > > [[nodiscard]] to be allowed,
> >
> >
> > You can use `[[clang::warn_unused_result]]` for this evaluation, that does 
> > not require C++17.
> >
> > > But the clang-tidy -fix doesn't break the build, the patch is large but 
> > > nothing broke. (at compile time at least!)
> > > 
> > >   {F7661182} 
> >
> > Uhm, so there wasn't a single true-positive in protobuf?
>
>
> To elaborate, what i'm asking is:
>
> - Is that project -Werror clean without those changes?
> - if yes, after all those changes, does the -Werror build break?
>   - If it does break, how many of those issues are actual bugs (ignored 
> return when it should not be ignored), and how many are noise.
>   - If not, then i guess all these were "false-positives"


I totally get where you are coming from, and I want to answer correctly...

1. protobuf was clean to begin with when compiling with -Werror
2. after applying nodiscard fix-it protobuf would not build cleanly with 
-Werror but will build cleanly without it
3. those warnigns ARE related to the introduction of the [[nodiscard]]

  F7673134: image.png 

taking the first one as an example, I'm trying to determine if

  int ExtensionSet::NumExtensions() const {
int result = 0;
ForEach([](int /* number */, const Extension& ext) {
  ^^^
  if (!ext.is_cleared) {
++result;
  }
});
return result;
  }

having the checker added nodiscard..

  template 
  [[nodiscard]] KeyValueFunctor ForEach(KeyValueFunctor func) const {
if (PROTOBUF_PREDICT_FALSE(is_large())) {
  return ForEach(map_.large->begin(), map_.large->end(), std::move(func));
}
return ForEach(flat_begin(), flat_end(), std::move(func));
  }

Is a false positive, or is Visual C++ somehow thinking the return value is not 
used when its a lambda? but I get similar issues when compiling with the Intel 
compiler, so I think its more likely that the return value from ForEach isn't 
actually used, and the implementation is just allowing ForEach()'s to be 
chained together

F7673175: image.png 

All of these new warnings introduced are related to the use of a lambda with 
the exception of

  // We don't care what this returns since we'll find out below anyway.
  pool_->TryFindFileInFallbackDatabase(proto.dependency(i));

Which you could say that it should be written as

  [[maybe_unused]] auto rt= 
pool_->TryFindFileInFallbackDatabase(proto.dependency(i));

But the effect of applying the fix-it, doesn't itself break the build, its the 
effect of [[nodiscard]] having been added. (perhaps incorrectly)

However the build is not broken when NOT using warnings as errors, and so it 
depends on the "ethos" of clang-tidy, if the goal is for clang-tidy to remain 
error/warning free after applying -fix then i can see the fix-it generating 
changes that causes warnings isn't correct and that should be considered a 
false positive and I should consider not emitting the [[nodiscard]] when the 
argument is perhaps a lambda.

I guess in the past I've used clang-tidy to also help me generate new warnings 
so I can fix that code, but that may not be the goal of its usage.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

MyDeveloperDay wrote:
> curdeius wrote:
> > curdeius wrote:
> > > Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
> > It might have been discussed already, but is it a common convention to mark 
> > internal functions with a leading underscore?
> > If that comes from system headers, AFAIK clang-tidy already is able not to 
> > emit warnings from them.
> If you feel this is an unnecessary option, I'm happy to remove it, it might 
> simplify things.
on reflection as both @curdeius  and other reviews expressed concern of 
"isInternalFunction" motivation, I decided to remove this as an option, I think 
I was letting my usecase get mixed up with the design


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+ return doesFunctionNameStartWithUnderScore();

curdeius wrote:
> If think that you should run clang-format over your patch.
Sorry my bad, I do have clang-format to run on save which should pick up your 
style, but I forgot you guys don't like braces on small ifs...



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

curdeius wrote:
> curdeius wrote:
> > Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
> It might have been discussed already, but is it a common convention to mark 
> internal functions with a leading underscore?
> If that comes from system headers, AFAIK clang-tidy already is able not to 
> emit warnings from them.
If you feel this is an unnecessary option, I'm happy to remove it, it might 
simplify things.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177474.
MyDeveloperDay marked 11 inline comments as done.
MyDeveloperDay added a comment.

- Addressing review comments and concerns
- Removed internal function logic and option, not really the role of this 
checker
- Fixed grammatical error in documentation
- enabled fix-it check in unit tests for  already present [[nodiscard]] by 
using escaping which allowed use of [[]] in CHECK-FIXES
- minor clang format issue


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.cpp

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: :[[@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: 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D55433#1323779 , @lebedev.ri wrote:

> In D55433#1323757 , @MyDeveloperDay 
> wrote:
>
> > a lot of projects aren't setup for c++17 yet which is needed for 
> > [[nodiscard]] to be allowed,
>
>
> You can use `[[clang::warn_unused_result]]` for this evaluation, that does 
> not require C++17.
>
> > But the clang-tidy -fix doesn't break the build, the patch is large but 
> > nothing broke. (at compile time at least!)
> > 
> >   {F7661182} 
>
> Uhm, so there wasn't a single true-positive in protobuf?


To elaborate, what i'm asking is:

- Is that project -Werror clean without those changes?
- if yes, after all those changes, does the -Werror build break?
  - If it does break, how many of those issues are actual bugs (ignored return 
when it should not be ignored), and how many are noise.
  - If not, then i guess all these were "false-positives"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Just a few minor remarks and a possible workaround for testing `CHECK-FIXES: 
[[nodiscard]]`.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+ return doesFunctionNameStartWithUnderScore();

If think that you should run clang-format over your patch.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:118
+  // 2. a const member function which return a variable which is ignored
+  // performs some externation io operation and the return value could be 
ignore
+

Please fix typos/grammar, externation -> external, io -> I/O, ignore -> ignored.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Users can use :option:`ReplacementString` to specify a macro to use instead of
+``[[nodiscard]]``. This is useful when maintaining source code that needs to

I would avoid repeating the option name in its description and state clearly 
that `[[nodiscard]]` is the default.

Cf. other clang-tidy checks with options: 
http://clang.llvm.org/extra/clang-tidy/checks/list.html, e.g. 
http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html#options.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:39
+``[[nodiscard]]``. This is useful when maintaining source code that needs to
+also compile with a non c++17 conforming compiler.
+

I'd suggest "needs to compile with a pre-C++17 compiler."



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()

"turn off the adding"? I'm not a native English speaker, but "turn off addition 
of" would sound better.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()

curdeius wrote:
> "turn off the adding"? I'm not a native English speaker, but "turn off 
> addition of" would sound better.
Same as for the other option, I wouldn't repeat the option name and give the 
default value explicitly.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

curdeius wrote:
> Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
It might have been discussed already, but is it a common convention to mark 
internal functions with a leading underscore?
If that comes from system headers, AFAIK clang-tidy already is able not to emit 
warnings from them.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;

I think you can use a regex as a workaround, i.e. using `{{\[\[nodiscard\]\]}}`.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:66
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+

So this line should become: `// CHECK-FIXES: {{\[\[nodiscard\]\] bool f11() 
const;`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177330.
MyDeveloperDay added a comment.

Minor alterations to Release notes and Documentation to address review comments


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.cpp

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
+// 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177327.
MyDeveloperDay added a comment.

- Move the final conditional statements into AST_MATCHERS
- Add additional IgnoreInternalFunctions option to allow adding of 
``[[nodiscard]]`` to functions starting with _ (e.g. _Foo())


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.cpp

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 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:173
+
+  This check adds ``[[nodiscard]]`` attributes (introduced in C++17) to member
+  functions to highlight at compile time where the return value of a function

Please omit this check. Same in documentation.



Comment at: docs/ReleaseNotes.rst:175
+  functions to highlight at compile time where the return value of a function
+  should not be ignored
+

Please add dot at the end.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177313.
MyDeveloperDay added a comment.

Move the conditional statements into AST_MATCHERS


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.cpp

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:
+// 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177312.
MyDeveloperDay added a comment.

Move the conditional checks into matchers


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.cpp

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:
+// 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177311.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Move more of the conditional checks into the matchers


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.cpp

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() 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 12 inline comments as done.
MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:28-30
+bool isOperator(const FunctionDecl *D) {
+  return D->getNameAsString().find("operator") != std::string::npos;
+}

lebedev.ri wrote:
> Can't you use `isOverloadedOperator()`?
> No way going to `std::string` is fast.
Great! I didn't even know that existed (this is my first checker so I'm still 
learning what I can do), I tried to use it in the matcher but I couldn't get it 
to work, (see comment below about your matcher comment)



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32-34
+bool isInternalFunction(const FunctionDecl *D) {
+  return D->getNameAsString().find("_") == 0;
+}

lebedev.ri wrote:
> Motivation?
> This should be configurable in some way.
this was because when I ran it on my source tree with -fix it, it wanted to try 
and fix the windows headers, and that scared me a bit! so I was trying to 
prevent it stepping into standard headers etc.. I'll make a change to make that 
optional



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:47
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+return;

lebedev.ri wrote:
> This should be smarter, since you provide `ReplacementString`.
> E.g. if `ReplacementString` is default, require C++17.
> Else, only require C++.
That's a great idea, my team can barely get onto all of C++ 11 because some 
platform compilers are SO far behind, but I really like that idea of allowing 
it to still work if using a macro



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:63-95
+  // if the location is invalid forget it
+  if (!MatchedDecl->getLocation().isValid())
+return;
+
+  if (MatchedDecl->isThisDeclarationADefinition() && 
MatchedDecl->isOutOfLine())
+return;
+

lebedev.ri wrote:
> All this should be done in `registerMatchers()`.
Ok, I think this is my naivety on how to actually do it, because I tried but 
often the functions on MatchDecl->isXXX() didn't always map 1-to-1 with what 
the matchers were expecting (but most likely I was just doing it wrong), let me 
swing back round once I've read some more from @stephenkelly blogs



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:108
+  << MatchedDecl
+  << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+  return;

lebedev.ri wrote:
> Can you actually provide fix-it though?
> If the result will be unused, it will break build under `-Werror`.
It is true, it will generate warnings if people aren't using it, does that go 
against what people traditionally believe clang-tidy should do? I mean I get it 
that clang-tidy should mostly tidy the code and leave it in a working state, 
but is causing people to break their eggs considered a no-no? again this might 
be my naivety about what clang-tidy remit is.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:6
+
+This 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
+

lebedev.ri wrote:
> Eugene.Zelenko wrote:
> > Please use 80 characters limit.
> Same, the rules need to be spelled out, this sounds a bit too magical right 
> now,
I'm not a great wordsmith..but is this too wordy for what you like in the 
Docs/ReleaseNotes?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:40
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+
+  // if we are using C++17 attributes we are going to need c++17

Unnecessary empty line.



Comment at: docs/ReleaseNotes.rst:173
+
+  Checks to detect if a member function looks like its return type should not 
+  be ignored.

One sentence is enough for Release Notes. Will be good idea to make it same as 
in documentation.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:40
+
+Users can use :option:`ReplacementString` to specify a macro to use instead of
+``[[nodiscard]]``. This is useful when maintaining source code that needs to

May be just this option?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 177295.
MyDeveloperDay added a comment.

Address some (not all yet) review comments


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.cpp

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:
+// 

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D55433#1323757 , @MyDeveloperDay 
wrote:

> a lot of projects aren't setup for c++17 yet which is needed for 
> [[nodiscard]] to be allowed,


You can use `[[clang::warn_unused_result]]` for this evaluation, that does not 
require C++17.

> But the clang-tidy -fix doesn't break the build, the patch is large but 
> nothing broke. (at compile time at least!)
> 
>   {F7661182} 

Uhm, so there wasn't a single true-positive in protobuf?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm trying to generate an example of the modernize-use-nodiscard checker on 
something open source...as I developed this checker on Windows I struggled a 
little getting cmake to build me the json file to run clang-tidy over 
everything and a lot of projects aren't setup of c++17 yet which is needed for 
[[nodiscard]] to be allowed,  but using ClangPowerTools inside Visual studio I 
decided to run it on **protobuf**

I ran "Tidy Fix" with just the following custom checks 
"-*,modernize-use-nodiscard" this will fix some of the headers (alot of the 
headers actually)

recompiling the project, I see this one, which made me laugh...

  protobuf\protobuf\src\google\protobuf\descriptor.cc(4238): warning C4834: 
discarding return value of function with 'nodiscard' attribute

from descriptor.h I can see its fixed the header adding [[nodiscard]]'s

  // Tries to find something in the fallback database and link in the
  // corresponding proto file.  Returns true if successful, in which case
  // the caller should search for the thing again.  These are declared
  // const because they are called by (semantically) const methods.
  [[nodiscard]] bool TryFindFileInFallbackDatabase(const std::string& name) 
const;
  [[nodiscard]] bool TryFindSymbolInFallbackDatabase(const std::string& name) 
const;

ironically there is a comment in the code...

if (tables_->FindFile(proto.dependency(i)) == NULL &&
(pool_->underlay_ == NULL ||
 pool_->underlay_->FindFileByName(proto.dependency(i)) == NULL)) {
  // We don't care what this returns since we'll find out below anyway.
  pool_->TryFindFileInFallbackDatabase(proto.dependency(i));
  ^^
}
  }
  tables_->pending_files_.pop_back();

For me its as much about why the comment was put there, someone was having to 
explain why they didn't care about the return, but this could easily have been 
a bug

Bug the clang-tidy -fix doesn't break the build, the patch is large but nothing 
broke.

F7661182: protobuf.patch 


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:28-30
+bool isOperator(const FunctionDecl *D) {
+  return D->getNameAsString().find("operator") != std::string::npos;
+}

Can't you use `isOverloadedOperator()`?
No way going to `std::string` is fast.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:32-34
+bool isInternalFunction(const FunctionDecl *D) {
+  return D->getNameAsString().find("_") == 0;
+}

Motivation?
This should be configurable in some way.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:47
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+return;

This should be smarter, since you provide `ReplacementString`.
E.g. if `ReplacementString` is default, require C++17.
Else, only require C++.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:63-95
+  // if the location is invalid forget it
+  if (!MatchedDecl->getLocation().isValid())
+return;
+
+  if (MatchedDecl->isThisDeclarationADefinition() && 
MatchedDecl->isOutOfLine())
+return;
+

All this should be done in `registerMatchers()`.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:108
+  << MatchedDecl
+  << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+  return;

Can you actually provide fix-it though?
If the result will be unused, it will break build under `-Werror`.



Comment at: docs/ReleaseNotes.rst:79-81
+  Checks for non void const member functions which should have 
+  ``[[nodiscard]]`` added to tell the compiler a return type shoud not be 
+  ignored

The actual rules should be spelled out here, right now this sounds a bit too 
magical.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:6
+
+This 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
+

Eugene.Zelenko wrote:
> Please use 80 characters limit.
Same, the rules need to be spelled out, this sounds a bit too magical right now,


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I recently had trouble with ignoring the return type, I would certainly 
appreciate a tool like this. Can you test it on LLVM+Clang? Maybe some methods 
in the AST library could also benefit from `LLVM_NODISCARD` (containers with 
factories is my no1 thought), and it's also a large enough codebase that it 
would be representative as to how the check behaves.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D55433#1323145 , @MyDeveloperDay 
wrote:

> In D55433#1323106 , @lebedev.ri 
> wrote:
>
> > Have you evaluated this on some major C++ projects yet?
> >  I suspect this may have pretty low SNR.
>
>
> Internally yes (on millions of lines), but you are correct high noise level,


... and the examples of the most common noise are?
Please run it on some codebase (llvm?) with run-clang-tidy.py, and post the 
results.

> however resulting compiles showed a number of hard to find bugs most notably 
> where empty()  function on custom container was being used when they meant 
> clear(), this bug had gone undetected for 10+ years.

That does sound good.

> The idea came from guidance given in Jason Turner @lefticus  CppCon2018 talk, 
> https://www.youtube.com/watch?v=DHOlsEd0eDE (about 14 minutes in)
> 
> "is it a logical error in your code to call this member function and discard 
> the return value"... the room says yes!!




Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:21
+namespace modernize {
+namespace {
+

Please use static instead anonymous namespace for functions. See LLVM code 
style guidelines.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:103
+
+  auto retLoc = MatchedDecl->getInnerLocStart();
+

Please don't use auto when type could not be deducted from same statement.



Comment at: docs/ReleaseNotes.rst:76
 
+- New :doc:`modernize-use-nodiscard
+  ` check.

Please use alphabetical order for new checks list.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:6
+
+This 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
+

Please use 80 characters limit.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55433#1323106 , @lebedev.ri wrote:

> Have you evaluated this on some major C++ projects yet?
>  I suspect this may have pretty low SNR.


Internally yes (on millions of lines), but you are correct high noise level, 
however resulting compiles showed a number of hard to find bugs most notably 
where empty()  function on custom container was being used when they meant 
clear(), this bug had gone undetected for 10+ years.

The idea came from guidance given in Jason Turner CppCon2018 talk, 
https://www.youtube.com/watch?v=DHOlsEd0eDE (about 14 minutes in)

"is it a logical error in your code to call this member function and discard 
the return value"... the room says yes!!


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Have you evaluated this on some major C++ projects yet?
I suspect this will have pretty low SNR.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

[Paul Hoad via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: alexfh, stephenkelly, curdeius, aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

Adds a checker to clang-tidy to warn when a non void const member function, 
taking only parameters passed by value or const reference could be marked as 
'[[nodiscard]]'


Repository:
  rCTE Clang Tools Extra

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.cpp

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