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

[MyDeveloperDay]

Summary

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]]'

[curdeius]

clang-tidy/modernize/UseNodiscardCheck.cpp

43 ↗

(On Diff #177330)

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

docs/clang-tidy/checks/modernize-use-nodiscard.rst

37 ↗

(On Diff #177330)

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.

39 ↗

(On Diff #177330)

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

61 ↗

(On Diff #177330)

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

61 ↗

(On Diff #177330)

Same as for the other option, I wouldn't repeat the option name and give the default value explicitly.

62 ↗

(On Diff #177330)

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

62 ↗

(On Diff #177330)

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.

test/clang-tidy/modernize-use-nodiscard.cpp

63 ↗

(On Diff #177330)

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

66 ↗

(On Diff #177330)

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

[lebedev.ri]

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

protobuf.patch701 KBDownload

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"

[MyDeveloperDay]

• 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

clang-tidy/modernize/UseNodiscardCheck.cpp

43 ↗

(On Diff #177330)

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

docs/clang-tidy/checks/modernize-use-nodiscard.rst

62 ↗

(On Diff #177330)

If you feel this is an unnecessary option, I'm happy to remove it, it might simplify things.

[MyDeveloperDay]

docs/clang-tidy/checks/modernize-use-nodiscard.rst

62 ↗

(On Diff #177330)

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

[MyDeveloperDay]

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

protobuf.patch701 KBDownload

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]]
   
   
   

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

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

having the checker added nodiscard..

template <typename KeyValueFunctor>
[[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

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.

[Eugene.Zelenko]

docs/clang-tidy/checks/modernize-use-nodiscard.rst

37 ↗

(On Diff #177474)

Specifies. Please wrap up to 80 characters.

[MyDeveloperDay]

Update the documentation to utilize 80 character lines

[Eugene.Zelenko]

docs/clang-tidy/checks/modernize-use-nodiscard.rst

37 ↗

(On Diff #177496)

Option specifies. Please fix double space.

[curdeius]

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.

clang-tidy/modernize/UseNodiscardCheck.cpp

45 ↗

(On Diff #177474)

Wouldn't something along the lines:

const auto &parameters = 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 &ParType = Par->getType(); // not sure about Type

if (isNonConstReferenceType(ParType)) {
// ...
if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() possible?
// ...

but that's a matter of taste.

docs/clang-tidy/checks/modernize-use-nodiscard.rst

12 ↗

(On Diff #177474)

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

test/clang-tidy/modernize-use-nodiscard.cpp

3 ↗

(On Diff #177474)

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.

63 ↗

(On Diff #177474)

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

[curdeius]

clang-tidy/modernize/UseNodiscardCheck.cpp

45 ↗

(On Diff #177474)

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 &ParType = Par->getType();

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

[MyDeveloperDay]

Addressing review comments,

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

[curdeius]

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.

[MyDeveloperDay]

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

[JonasToth]

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.

clang-tidy/modernize/UseNodiscardCheck.cpp

29 ↗

(On Diff #177532)

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

45 ↗

(On Diff #177532)

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

46 ↗

(On Diff #177532)

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.

48 ↗

(On Diff #177532)

You can ellide the braces.

95 ↗

(On Diff #177532)

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

104 ↗

(On Diff #177532)

you can remove that empty line.

clang-tidy/modernize/UseNodiscardCheck.h

19 ↗

(On Diff #177532)

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.

docs/ReleaseNotes.rst

174 ↗

(On Diff #177532)

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

docs/clang-tidy/checks/list.rst

15 ↗

(On Diff #177532)

please remove the spurious changes here.

docs/clang-tidy/checks/modernize-use-nodiscard.rst

38 ↗

(On Diff #177532)

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.

test/clang-tidy/modernize-use-nodiscard.cpp

9 ↗

(On Diff #177532)

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

[MyDeveloperDay]

Addressing review comments

• grammatical errors in documentation and comments
• prevent adding [[nodiscard]] to macros
• clean up list.rst

[Eugene.Zelenko]

docs/clang-tidy/checks/modernize-use-nodiscard.rst

49 ↗

(On Diff #177584)

Unnecessary empty line.

[JonasToth]

clang-tidy/modernize/UseNodiscardCheck.cpp

29 ↗

(On Diff #177584)

s/front/in front/

45 ↗

(On Diff #177584)

please remove the reference.

66 ↗

(On Diff #177584)

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

105 ↗

(On Diff #177584)

this return is not necessary

test/clang-tidy/modernize-use-nodiscard.cpp

11 ↗

(On Diff #177584)

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

126 ↗

(On Diff #177584)

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.

[curdeius]

A few more ideas for enhancements.

clang-tidy/modernize/UseNodiscardCheck.cpp

41 ↗

(On Diff #177584)

Double space after comma. Please check all comments.

docs/clang-tidy/checks/modernize-use-nodiscard.rst

9 ↗

(On Diff #177584)

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

test/clang-tidy/modernize-use-nodiscard-cxx11.cpp

24 ↗

(On Diff #177584)

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

test/clang-tidy/modernize-use-nodiscard.cpp

126 ↗

(On Diff #177584)

It might be a good idea to add a note in the documentation about handling of function templates and functions in class templates.

[curdeius]

clang-tidy/modernize/UseNodiscardCheck.cpp

65 ↗

(On Diff #177584)

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

73 ↗

(On Diff #177584)

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

92 ↗

(On Diff #177584)

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

[MyDeveloperDay]

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

[MyDeveloperDay]

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 &SM,
                                           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<Cursor> 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.

test/clang-tidy/modernize-use-nodiscard.cpp

126 ↗

(On Diff #177584)

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

[JonasToth]

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 &SM,
                                           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<Cursor> 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.

[JonasToth]

test/clang-tidy/modernize-use-nodiscard.cpp

126 ↗

(On Diff #177584)

Is this resolved, as you marked it done?

[JonasToth]

test/clang-tidy/modernize-use-nodiscard.cpp

183 ↗

(On Diff #177679)

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.

[MyDeveloperDay]

unsigned BlockOrCode = 0;
llvm::ErrorOr<Cursor> 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

clang-tidy/modernize/UseNodiscardCheck.cpp

73 ↗

(On Diff #177584)

merged into one if as suggested by @JonasToth

test/clang-tidy/modernize-use-nodiscard.cpp

183 ↗

(On Diff #177679)

so keep the NOT: warning:" line and remove the CHECK-FIXES lines (ok will do that)

126 ↗

(On Diff #177584)

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

[MyDeveloperDay]

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

[MyDeveloperDay]

test/clang-tidy/modernize-use-nodiscard.cpp

126 ↗

(On Diff #177584)

template arguments are now ignored so these tests are fixed

[MyDeveloperDay]

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

[MyDeveloperDay]

test/clang-tidy/modernize-use-nodiscard.cpp

126 ↗

(On Diff #177584)

I found detecting typename T::value_type and const_reference, particularly troublesome, in the end opting for the following, which I wasn't too happy with

static bool isAliasedTemplateParamType(const QualType &ParamType) {
  return (ParamType.getCanonicalType().getAsString().find("type-parameter-") !=
          std::string::npos);
}

I'm not sure if there is a better way?

[curdeius]

clang-tidy/modernize/UseNodiscardCheck.cpp

45 ↗

(On Diff #178123)

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

docs/clang-tidy/checks/modernize-use-nodiscard.rst

23 ↗

(On Diff #178123)

Typo: alteringing

[curdeius]

clang-tidy/modernize/UseNodiscardCheck.cpp

22 ↗

(On Diff #178123)

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

25 ↗

(On Diff #178123)

This indeed looks a bit ugly. Is there no check that skips const-ref template params and handles usings / typedefs?

29 ↗

(On Diff #178123)

TODO: too.

31 ↗

(On Diff #178123)

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.

test/clang-tidy/modernize-use-nodiscard.cpp

162 ↗

(On Diff #178123)

Could you use similar test cases for typedefs, please?

[MyDeveloperDay]

Fix review comments

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

[MyDeveloperDay]

clang-tidy/modernize/UseNodiscardCheck.cpp

25 ↗

(On Diff #178123)

I'm half hoping @JonasToth might guide me to something better ;-)

31 ↗

(On Diff #178123)

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

[MyDeveloperDay]

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

[MyDeveloperDay]

docs/clang-tidy/checks/modernize-use-nodiscard.rst

23 ↗

(On Diff #178355)

noted > 80 char line will correct next patch

[MyDeveloperDay]

-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

[curdeius]

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.

clang-tidy/modernize/UseNodiscardCheck.cpp

32 ↗

(On Diff #178367)

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?

127 ↗

(On Diff #178367)

existance -> existence

test/clang-tidy/modernize-use-nodiscard.cpp

231 ↗

(On Diff #178367)

Single 's' in [[nodiscard]].

[MyDeveloperDay]

Fix review comments

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

[MyDeveloperDay]

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.

[MyDeveloperDay]

Remove unused matcher variable

[MyDeveloperDay]

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

[JonasToth]

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!

[JonasToth]

clang-tidy/modernize/UseNodiscardCheck.cpp

24 ↗

(On Diff #179445)

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

25 ↗

(On Diff #179445)

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

30 ↗

(On Diff #179445)

Do you want a function-template (template <class T> 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.

32 ↗

(On Diff #179445)

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

41 ↗

(On Diff #179445)

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

53 ↗

(On Diff #179445)

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

105 ↗

(On Diff #179445)

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

114 ↗

(On Diff #179445)

misplaced comment

126 ↗

(On Diff #179445)

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

128 ↗

(On Diff #179445)

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

docs/ReleaseNotes.rst

26 ↗

(On Diff #179445)

spurious change

test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp

13 ↗

(On Diff #179445)

you can remove the CHECK-FIXES as there is nothing changed. If a change would be applied, FileCheck should notice that.

test/clang-tidy/modernize-use-nodiscard-no-macro.cpp

1 ↗

(On Diff #179445)

please merge these two lines.

test/clang-tidy/modernize-use-nodiscard.cpp

141 ↗

(On Diff #179445)

You dont need these lines, this happens implicitly in check_clang_tidy.py.

[MyDeveloperDay]

Rebase

• update to address most review comments from @JonasToth
• simplify matcher (with less unless)
• remove the need to look for "type-parameter-" string
• remove empty warning messages from tests (handled by FileCheck)

[MyDeveloperDay]

clang-tidy/modernize/UseNodiscardCheck.cpp

30 ↗

(On Diff #179445)

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

[JonasToth]

clang-tidy/modernize/UseNodiscardCheck.cpp

30 ↗

(On Diff #179445)

I experimented a bit and found something that might work for you. See the attachment.

test_function_has_name.cpp389 BDownload

[MyDeveloperDay]

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

[MyDeveloperDay]

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

[MyDeveloperDay]

clang-tidy/modernize/UseNodiscardCheck.cpp

30 ↗

(On Diff #179445)

This really helped thank you!

[JonasToth]

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

clang-tidy/modernize/UseNodiscardCheck.cpp

23 ↗

(On Diff #180212)

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.

111 ↗

(On Diff #180212)

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

128 ↗

(On Diff #180212)

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

130 ↗

(On Diff #180212)

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

auto Diag = diag(...);

if (canTransform)
   Diag << Change;

[MyDeveloperDay]

Update with view comments from @JonasToth

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

[MyDeveloperDay]

clang-tidy/modernize/UseNodiscardCheck.cpp

23 ↗

(On Diff #180212)

I agree, cleaner less code and easier to understand..moving it to a matcher

111 ↗

(On Diff #180212)

I had to move the hasType inside the anyOf?

130 ↗

(On Diff #180212)

that feels cleaner

[JonasToth]

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.

[MyDeveloperDay]

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

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

[JonasToth]

In D55433#1347553, @MyDeveloperDay wrote:

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

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?

[MyDeveloperDay]

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

[JonasToth]

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?

clang-tidy/modernize/UseNodiscardCheck.cpp

39 ↗

(On Diff #180625)

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

106 ↗

(On Diff #180625)

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

test/clang-tidy/modernize-use-nodiscard.cpp

141 ↗

(On Diff #180625)

typo, paramaeter

[MyDeveloperDay]

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

[MyDeveloperDay]

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

clang-tidy/modernize/UseNodiscardCheck.cpp

106 ↗

(On Diff #180625)

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

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!

[MyDeveloperDay]

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 []{... )

[MyDeveloperDay]

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

[JonasToth]

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

[MyDeveloperDay]

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.

[JonasToth]

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

[MyDeveloperDay]

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?

[JonasToth]

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

clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   ShrinkToFitCheck.cpp
   UnaryStaticAssertCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
+  UseNodiscardCheck.cpp
   UseNoexceptCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseTransparentFunctorsCheck.cpp
   UseUncaughtExceptionsCheck.cpp
   UseUsingCheck.cpp
 
   LINK_LIBS

clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "ShrinkToFitCheck.h"
 #include "UnaryStaticAssertCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
+#include "UseNodiscardCheck.h"
 #include "UseNoexceptCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseTransparentFunctorsCheck.h"
 #include "UseUncaughtExceptionsCheck.h"
 #include "UseUsingCheck.h"
 
 using namespace clang::ast_matchers;
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultMemberInitCheck>(
         "modernize-use-default-member-init");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseEqualsDefaultCheck>("modernize-use-equals-default");
     CheckFactories.registerCheck<UseEqualsDeleteCheck>(
         "modernize-use-equals-delete");
+    CheckFactories.registerCheck<UseNodiscardCheck>(
+        "modernize-use-nodiscard");
     CheckFactories.registerCheck<UseNoexceptCheck>("modernize-use-noexcept");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
     CheckFactories.registerCheck<UseTransparentFunctorsCheck>(
         "modernize-use-transparent-functors");
     CheckFactories.registerCheck<UseUncaughtExceptionsCheck>(
         "modernize-use-uncaught-exceptions");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");

clang-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.h

+//===--- UseNodiscardCheck.h - clang-tidy -----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USENODISCARDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USENODISCARDCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// \brief Add ``[[nodiscard]]`` to non-void const-member functions with no
+/// arguments or pass-by-value or pass by const-reference arguments.
+/// \code
+///    bool empty() const;
+///    bool empty(const Bar &) const;
+///    bool empty(int bar) const;
+/// \endcode
+/// Is converted to:
+/// \code
+///    [[nodiscard]] bool empty() const;
+///    [[nodiscard]] bool empty(const Bar &) const;
+///    [[nodiscard]] bool empty(int bar) const;
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nodiscard.html
+class UseNodiscardCheck : public ClangTidyCheck {
+public:
+  UseNodiscardCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::string NoDiscardMacro;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USENODISCARDCHECK_H

clang-tools-extra/trunk/clang-tidy/modernize/UseNodiscardCheck.cpp

+//===--- 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 &Context,
+                                    const llvm::StringRef &MacroId) {
+  // 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<CXXConversionDecl>(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, hasTemplateReturnType) {
+  // Don't put ``[[nodiscard]]`` in front of functions returning a template
+  // type.
+  return Node.getReturnType()->isTemplateTypeParmType() ||
+         Node.getReturnType()->isInstantiationDependentType();
+}
+AST_MATCHER(CXXMethodDecl, isDefinitionOrInline) {
+  // A function definition, with optional inline but not the declaration.
+  return !(Node.isThisDeclarationADefinition() && Node.isOutOfLine());
+}
+AST_MATCHER(QualType, isInstantiationDependentType) {
+  return Node->isInstantiationDependentType();
+}
+AST_MATCHER(QualType, isNonConstReferenceOrPointer) {
+  // If the function has any non-const-reference arguments
+  //    bool foo(A &a)
+  // or pointer arguments
+  //    bool foo(A*)
+  // then they may not care about the return value because of passing data
+  // via the arguments.
+  return (Node->isTemplateTypeParmType() || Node->isPointerType() ||
+          (Node->isReferenceType() &&
+           !Node.getNonReferenceType().isConstQualified()) ||
+          Node->isInstantiationDependentType());
+}
+} // namespace
+
+UseNodiscardCheck::UseNodiscardCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      NoDiscardMacro(Options.get("ReplacementString", "[[nodiscard]]")) {}
+
+void UseNodiscardCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "ReplacementString", NoDiscardMacro);
+}
+
+void UseNodiscardCheck::registerMatchers(MatchFinder *Finder) {
+  // If we use ``[[nodiscard]]`` attribute, we require at least C++17. Use a
+  // macro or ``__attribute__`` with pre c++17 compilers by using
+  // ReplacementString option.
+  if ((NoDiscardMacro == "[[nodiscard]]" && !getLangOpts().CPlusPlus17) ||
+      !getLangOpts().CPlusPlus)
+    return;
+
+  auto functionObj =
+      cxxRecordDecl(hasAnyName("::std::function", "::boost::function"));
+
+  // Find all non-void const methods which have not already been marked to
+  // warn on unused result.
+  Finder->addMatcher(
+      cxxMethodDecl(
+          allOf(isConst(), isDefinitionOrInline(),
+                unless(anyOf(
+                    returns(voidType()), isNoReturn(), isOverloadedOperator(),
+                    isVariadic(), hasTemplateReturnType(),
+                    hasClassMutableFields(), isConversionOperator(),
+                    hasAttr(clang::attr::WarnUnusedResult),
+                    hasType(isInstantiationDependentType()),
+                    hasAnyParameter(anyOf(
+                        parmVarDecl(anyOf(hasType(functionObj),
+                                          hasType(references(functionObj)))),
+                        hasType(isNonConstReferenceOrPointer()),
+                        hasParameterPack()))))))
+          .bind("no_discard"),
+      this);
+}
+
+void UseNodiscardCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXMethodDecl>("no_discard");
+  // Don't make replacements if the location is invalid or in a macro.
+  SourceLocation Loc = MatchedDecl->getLocation();
+  if (Loc.isInvalid() || Loc.isMacroID())
+    return;
+
+  SourceLocation retLoc = MatchedDecl->getInnerLocStart();
+
+  ASTContext &Context = *Result.Context;
+
+  auto Diag = diag(retLoc, "function %0 should be marked " + NoDiscardMacro)
+              << MatchedDecl;
+
+  // Check for the existence of the keyword being used as the ``[[nodiscard]]``.
+  if (!doesNoDiscardMacroExist(Context, NoDiscardMacro))
+    return;
+
+  // Possible false positives include:
+  // 1. A const member function which returns a variable which is ignored
+  // but performs some external I/O operation and the return value could be
+  // ignored.
+  Diag << FixItHint::CreateInsertion(retLoc, NoDiscardMacro + " ");
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New :doc:`modernize-concat-nested-namespaces
   syntax introduced in C++17 standard: ``namespace a::b { ... }``.
 
 - New :doc:`modernize-deprecated-ios-base-aliases
   <clang-tidy/checks/modernize-deprecated-ios-base-aliases>` check.
 
   Detects usage of the deprecated member types of ``std::ios_base`` and replaces
   those that have a non-deprecated equivalent.
 
+- New :doc:`modernize-use-nodiscard
+  <clang-tidy/checks/modernize-use-nodiscard>` check.
+
+  Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions
+  to highlight at compile time which return values should not be ignored.
+
 - New :doc:`readability-isolate-decl
   <clang-tidy/checks/readability-isolate-declaration>` check.
 
   Detects local variable declarations declaring more than one variable and
   tries to refactor the code to one statement per declaration.
 
 - New :doc:`readability-const-return-type
   <clang-tidy/checks/readability-const-return-type>` check.

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-shrink-to-fit
    modernize-unary-static-assert
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default-member-init
    modernize-use-emplace
    modernize-use-equals-default
    modernize-use-equals-delete
+   modernize-use-nodiscard
    modernize-use-noexcept
    modernize-use-nullptr
    modernize-use-override
    modernize-use-transparent-functors
    modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-nodiscard.rst

+.. title:: clang-tidy - modernize-use-nodiscard
+
+modernize-use-nodiscard
+=======================
+
+Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions in
+order to highlight at compile time which return values should not be ignored.
+
+Member functions need to satisfy the following conditions to be considered by
+this check:
+  - no ``[[nodiscard]]``, ``[[noreturn]]``, ``__attribute__((warn_unused_result))``, ``[[clang::warn_unused_result]]`` nor ``[[gcc::warn_unused_result]]`` attribute,
+  - non-void return type,
+  - non-template return types,
+  - const member function,
+  - non-variadic functions,
+  - no non-const reference parameters,
+  - no pointer parameters,
+  - no template parameters,
+  - no template function parameters,
+  - not be a member of a class with mutable member variables,
+  - no Lambdas,
+  - no conversion functions.
+
+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).
+
+Example
+-------
+
+.. code-block:: c++
+
+    bool empty() const;
+    bool empty(int i) const;
+
+transforms to:
+
+.. code-block:: c++
+
+    [[nodiscard] bool empty() const;
+    [[nodiscard] bool empty(int i) const;
+
+Options
+-------
+
+.. option:: ReplacementString
+
+Specifies a macro to use instead of ``[[nodiscard]]``. This is useful when
+maintaining source code that needs to compile with a pre-C++17 compiler.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+    bool empty() const;
+    bool empty(int i) const;
+
+transforms to:
+
+.. code-block:: c++
+
+    NO_DISCARD bool empty() const;
+    NO_DISCARD bool empty(int i) const;
+
+if the :option:`ReplacementString` option is set to `NO_DISCARD`.
+
+.. note::
+
+If the :option:`ReplacementString` is not a C++ attribute, but instead a macro,
+then that macro must be defined in scope or the fix-it will not be applied.
+
+.. note::
+
+   For alternative ``__attribute__`` syntax options to mark functions as
+   ``[[nodiscard]]`` in non-c++17 source code.
+   See https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-clang-unused.cpp

+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: '[[clang::warn_unused_result]]'}]}" \
+// RUN: -- -std=c++11 
+
+class Foo
+{
+public:
+    bool f1() const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f1() const;
+
+    bool f2(int) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f2(int) const;
+
+    bool f3(const int &) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f3(const int &) const;
+
+    bool f4(void) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked {{\[\[clang::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[clang::warn_unused_result\]\]}} bool f4(void) const;
+
+};
+

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-cxx11.cpp

+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: '__attribute__((warn_unused_result))'}]}" \
+// RUN: -- -std=c++11 \
+
+class Foo
+{
+public:
+    bool f1() const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard]
+    // CHECK-FIXES: __attribute__((warn_unused_result)) bool f1() const;
+
+    bool f2(int) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard]
+    // CHECK-FIXES: __attribute__((warn_unused_result)) bool f2(int) const;
+
+    bool f3(const int &) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard]
+    // CHECK-FIXES: __attribute__((warn_unused_result)) bool f3(const int &) const;
+
+    bool f4(void) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked __attribute__((warn_unused_result)) [modernize-use-nodiscard]
+    // CHECK-FIXES: __attribute__((warn_unused_result)) bool f4(void) const;
+};
+

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-gcc-unused.cpp

+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: '[[gcc::warn_unused_result]]'}]}" \
+// RUN: -- -std=c++11 
+
+class Foo
+{
+public:
+    bool f1() const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f1() const;
+
+    bool f2(int) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f2(int) const;
+
+    bool f3(const int &) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f3(const int &) const;
+
+    bool f4(void) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked {{\[\[gcc::warn_unused_result\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[gcc::warn_unused_result\]\]}} bool f4(void) const;
+
+};
+

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-no-macro-inscope-cxx11.cpp

+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'CUSTOM_NO_DISCARD'}]}" -- -std=c++11 
+
+// As if the macro was not defined.
+// #define CUSTOM_NO_DISCARD __attribute_((warn_unused_result))
+
+class Foo
+{
+public:
+    bool f1() const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked CUSTOM_NO_DISCARD [modernize-use-nodiscard]
+};
+

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard-no-macro.cpp

+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- -- -std=c++17 
+
+class Foo
+{
+public:
+    bool f1() const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f1() const;
+
+    bool f2(int) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f2(int) const;
+
+    bool f3(const int &) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f3(const int &) const;
+
+    bool f4(void) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked {{\[\[nodiscard\]\]}} [modernize-use-nodiscard]
+    // CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f4(void) const;
+
+};

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard.h

+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+

clang-tools-extra/trunk/test/clang-tidy/modernize-use-nodiscard.cpp

+// 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 <functional>
+
+namespace boost {
+template <class>
+class function;
+}
+
+#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;
+    
+    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 &f45() 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 &f45() 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;
+    
+    bool f24(size_type) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f24' should be marked NO_DISCARD [modernize-use-nodiscard]
+    // CHECK-FIXES: NO_DISCARD bool f24(size_type) const;
+    
+    bool f28(my_unsigned) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f28' should be marked NO_DISCARD [modernize-use-nodiscard]
+    // CHECK-FIXES: NO_DISCARD bool f28(my_unsigned) const;
+
+    bool f29(my_unsigned_reference) const;
+
+    bool f30(my_unsigned_const_reference) const;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f30' should be marked NO_DISCARD [modernize-use-nodiscard]
+    // CHECK-FIXES: NO_DISCARD bool f30(my_unsigned_const_reference) const;
+
+    template <class F>
+    F f37(F a, F b) const;
+
+    template <class F>
+    bool f38(F a) const;
+
+    bool f39(const std::function<bool()> &predicate) const;
+
+    bool f39a(std::function<bool()> predicate) const;
+
+    bool f39b(const std::function<bool()> predicate) const;
+
+    bool f45(const boost::function<bool()> &predicate) const;
+
+    bool f45a(boost::function<bool()> predicate) const;
+
+    bool f45b(const boost::function<bool()> predicate) const;
+
+    // Do not add ``[[nodiscard]]`` to parameter packs.
+    template <class... Args>
+    bool ParameterPack(Args... args) const;
+
+    template <typename... Targs>
+    bool ParameterPack2(Targs... Fargs) const;
+
+    // Do not add ``[[nodiscard]]`` to variadic functions.
+    bool VariadicFunctionTest(const int &, ...) const;
+
+    // Do not add ``[[nodiscard]]`` to non constant static functions.
+    static bool not_empty();
+
+    // Do not add ``[[nodiscard]]`` to conversion functions.
+    // explicit operator bool() const { return true; }
+};
+
+// Do not add ``[[nodiscard]]`` to Lambda.
+const auto nonConstReferenceType = [] {
+  return true;
+};
+
+auto lambda1 = [](int a, int b) { return a < b; };
+auto lambda1a = [](int a) { return a; };
+auto lambda1b = []()  { return true;};
+
+auto get_functor = [](bool check) {
+    return  [&](const std::string& sr)->std::string {
+        if(check){
+            return "some string";
+        }
+        return "another string";
+    };
+};
+
+// Do not add ``[[nodiscard]]`` to function definition.
+bool Foo::f19() const {
+  return true;
+}
+
+template <class T>
+class Bar {
+public:
+    using value_type = T;
+    using reference = value_type &;
+    using const_reference = const value_type &;
+
+    // Do not add ``[[nodiscard]]`` to non explicit conversion functions.
+    operator bool() const { return true; }
+
+    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;
+
+    bool f27(reference) const;
+
+    typename T::value_type f35() const;
+
+    T f34() const;
+
+    bool f31(T) const;
+
+    bool f33(T &) const;
+
+    bool f26(const_reference) const;
+
+    bool f32(const T &) const;
+};
+
+template <typename _Tp, int cn>
+class Vec {
+public:
+    Vec(_Tp v0, _Tp v1); //!< 2-element vector constructor
+
+    Vec cross(const Vec &v) const;
+
+    template <typename T2>
+    operator Vec<T2, cn>() const;
+};
+    
+template <class T>
+class Bar2 {
+public:
+  typedef T value_type;
+  typedef value_type &reference;
+  typedef const value_type &const_reference;
+
+  // we cannot assume that the template parameter isn't a pointer
+  bool f40(value_type) const;
+
+  bool f41(reference) const;
+
+  value_type f42() const;
+
+  typename T::value_type f43() const;
+
+  bool f44(const_reference) const;
+};
+
+template <class T>
+bool Bar<T>::empty() const {
+  return true;
+}
+
+// don't mark typical ``[[nodiscard]]`` candidates if the class
+// has mutable member variables
+class MutableExample {
+  mutable bool m_isempty;
+
+public:
+  bool empty() const;
+};