Why clang-tidy suggests to add [[nodiscard]] everywhere?

Question

I have a C++ project where clang-tidy is suggesting to add [[nodiscard]] everywhere. Is this a good practice ? The understanding I have is that [[nodiscard]] should be used only when ignoring the return value could be fatal for program. I have an object Car and it has a member const unsigned int m_ID. Should the getter unsigned int getID() have [[nodiscard]] ? clang-tidy suggests so.

EDIT:

Of course, I do not want to ignore a getter. BUT
My point is if every function that returns something should have a [[nodiscard]], then the attribute [[nodiscard]] is anyway redundant. Compiler can simply check all functions that return something.

Why would you want to ignore the value returned by a getter function? — Adrian Mole, Apr 12 '21 at 14:05
Yes it should have `[[nodiscard]]`, because it wouldn't make any sense to discard the return value of a getter and it's probably a bug if you do. — Stefan Riedel, Apr 12 '21 at 14:07
I don't think the rule make sense, `[[nodiscard]]` should only be used on those that actually should not be discard. (otherwise it provide literally no meaning since all function have it) — apple apple, Apr 12 '21 at 14:07
@AdrianMole I would not want to ignore the value returned by a getter. But given the fact that it is a getter, I would call only only when I am using `ID`. My point is ignoring the value returned by getter would have no impact on program. — Aditya Singh Rathore, Apr 12 '21 at 14:08
@AdityaSinghRathore The compiler is stopping you from making a mistake - this is a good thing yes ? _"... would have no impact on program..."_ so why even call the function - that's the mistake. — Richard Critten, Apr 12 '21 at 14:09
for example, when return a scope guard (e.g. lock) you'd not want it to be ignore. — apple apple, Apr 12 '21 at 14:10
The purpose of `[[nodiscard]]` is not just to prevent that you forget to handle the returned value (because it is somehow important) but also to shout at you if you write stupid code. Why the hell would you ignore the returned value of a getter? — Stefan Riedel, Apr 12 '21 at 14:10
@StefanRiedel My point is that since it is a getter, I will call it only when I need to get ID. Having a statement like `myCar->getID();` is not going to break the program. Why have unnecessasry [[nodiscard]] — Aditya Singh Rathore, Apr 12 '21 at 14:10
It is not only for when ignoring the return value would be "fatal". It is for when ignoring the return value doesn't make any sense and is likely a bug. If the only effect a method or function has is it's return value, then the only sensible options is to take the return value or not call the function at all. — Mikael Öhman, Apr 12 '21 at 14:10
@AdityaSinghRathore because all good programmers still make mistakes and it's nice to have the compiler catch as many as possible. — Richard Critten, Apr 12 '21 at 14:11
@AdityaSinghRathore right, you would only call that getter if you want to use that value. `[[nodiscard]]` will help you not mistakenly using a getter when all you wanted is using some modifying method. I've seen code where someone wanted to clear a vector using `myvec.empty()`. `[[nodiscard]]` would have helped here — Stefan Riedel, Apr 12 '21 at 14:14
@MikaelÖhman: "*It is for when ignoring the return value doesn't make any sense and is likely a bug.*" The C++ committee doesn't seem to agree. — Nicol Bolas, Apr 12 '21 at 14:15
@StefanRiedel @RichardCritten My point is if every function that returns something should have a [[nodiscard]], then the attribute `[[nodiscard]]` is anyway redundant. Compiler can simply check all functions that return something. — Aditya Singh Rathore, Apr 12 '21 at 14:16
@AdityaSinghRathore No one said "every function returning a value should have [[nodiscard]]". Just every function where discarding the returned value makes absolutely no sense should have [[nodiscard]] — Stefan Riedel, Apr 12 '21 at 14:17
@StefanRiedel I think deciding what are those functions is difficult for `clang-tidy` just based on declarations and usage. — Aditya Singh Rathore, Apr 12 '21 at 14:18
@AdityaSinghRathore Some functions have return values that are not necessarily useful. For example `erase` on a container returns an iterator that is not always useful. It would have been better design to have a `[[discardable]]` attribute and complain by default when a return value is ignored, but for backwards compatibility reasons this is not practical. — François Andrieux, Apr 12 '21 at 14:19
@AdityaSinghRathore Yes it's quite difficult for clang-tidy to decide just by a declaration. In fact it can't. But in case you're writing a getter method, it should have [[nodiscard]] — Stefan Riedel, Apr 12 '21 at 14:21
@FrançoisAndrieux: In fact it depends of point of view on how that attribute should be used. Standard (expect to) uses it only at few places (See Nicol Bolas' answer). In that context, it is the correct attribute. Other people would indeed prefer `[[discardable]]` attribute (as often, default is not the one we might expect at the end (`constexpr` everywhere, mutable/const, ...) :-( ) — Jarod42, Apr 12 '21 at 14:44

Nicol Bolas · Accepted Answer · 2021-04-12T14:19:04.353

This option is apparently "modernize-use-nodiscard", so you can deactivate that if you prefer.

It should be noted that the rules this option outlines are not the rules the C++ standard committee themselves use for when to apply [[nodiscard]]. Those rules being:

It should be added where:

For existing API’s

not using the return value always is a “huge mistake” (e.g. always resulting in resource leak)

not using the return value is a source of trouble and easily can happen (not obvious that something is wrong)

For new API’s (not been in the C++ standard yet)

not using the return value is usually an error.

It should not be added when:

For existing API’s

not using the return value is a possible/common way of programming at least for some input

for example for realloc(), which acts like free when the new site[sic] is 0

not using the return value makes no sense but doesn’t hurt and is usually not an error (e.g., because programmers meant to ask for a state change).

it is a C function, because their declaration might not be under control of the C++ implementation

This is why functions like operator new are [[nodiscard]], while functions like optional::value are not. There is a difference between being your code having a minor mistake and your code being fundamentally broken. [[nodiscard]], as far as the committee is concerned, is for the latter.

Note that container empty methods are a special case. They seem to fit the "do not use [[nodiscard]]" pattern, but because the name of empty is similar to the name for clear, if you don't use the return value of empty, odds are good that you meant to call clear.

Obviously, this cannot be known from just a declaration, so there's no way for Clang-Tidy to implement said rules.

eerorika · Answer 2 · 2021-04-12T14:35:30.590

Why clang-tidy suggests to add [[nodiscard]] everywhere?

clang-tidy doesn't suggest to add [[nodiscard]] everywhere. The cases where it is suggested are described in the documentation of the check.

Is this a good practice ?

Yes, using [[nodiscard]] is a good practice when discarding the result is likely a bug. That is the case quite often.

Should the getter unsigned int getID() have [[nodiscard]] ?

Can you imagine any use case where it would be useful to call that getter without using the returned value? If you are certain that such case won't exist, then you should use [[nodiscard]]. I think such case doesn't exist in the described example.

The understanding I have is that [[nodiscard]] should be used only when ignoring the return value could be fatal for program.

That's a rather conservative understanding. You can disable the check in question if you don't agree with it.

Why clang-tidy suggests to add [[nodiscard]] everywhere?

2 Answers2