36

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.

Improve this question
19
  • 1
    Why would you want to ignore the value returned by a getter function? Commented Apr 12, 2021 at 14:05
  • 2
    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. Commented Apr 12, 2021 at 14:07
  • 2
    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) Commented Apr 12, 2021 at 14:07
  • 2
    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? Commented Apr 12, 2021 at 14:10
  • 1
    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. Commented Apr 12, 2021 at 14:10

3 Answers 3

Reset to default
38

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

The rules stated lack "Do not add for new apis when not using the return value does not hurt" (e.g. getter-methods or optional::value()). You might say this is implicit, but they wrote it down for existing APIs which is also implicit by above rules...
@kuga: The Clang rules don't state that, but the C++ standard committees rules do: "It should not be added when: ... not using the return value makes no sense but doesn’t hurt and is usually not an error"
Correct. But ...only for existing apis. I just wanted to emphasize that does not hurt rule also applies to new apis, because this was subject to discussions in our team. It is just not explicitly written down.
10

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.

Improve this answer

Comments

1

The modernize-use-nodiscard checker was a checker I added to clang-tidy. I wanted my team to use [[nodiscard]] liberally to catch cases where people were accidentally using a function called empty() when they meant to use clear().

Because developers were coding like:

list.empty()

the presence of [[nodiscard]] on empty() forced the catching of such bugs. Once you start down this road you begin to see other places where people make mistakes, especially in a scenario where an api gets changed and now it returns something that should be handled.

Anything that can help a compiler to catch unintentional behavior is valuable as it left shifts bugs.

Improve this answer

3 Comments

You can also use verb prefixes (is, must, should, etc...) like many other languages do. If your method is named isEmpty() there's no more confusion.
I agree its good practice but sometimes you need the class to conform to the STL container interface so you can use your container as a drop in replacement, hence we don't always have the luxury of adding isXXXX in front of it.. hence the use of the attribute to catch when people don't read the return value. In fact there are many cases where a function is const, take no reference or pointer argument and doesn't touch any mutable members that a function can be considered that it needs nodiscard. (including function that begin with isXXXX() that can help as naming alone doesn't always
Yep, each project has its own requirements, I just added another possibility 😉

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.