Increasing quality: Always [[nodiscard]]

What is [[nodiscard]]?

https://en.cppreference.com/w/cpp/language/attributes/nodiscard

The [[nodiscard]] attribute introduced in C++17 will have the developer be notified with a warning when a function’s return value is discarded and either the function, or the struct/class type of the return value are attributed with [[nodiscard]]
(Remarks: Compilers are encouraged to emit a warning)

Let’s see some examples:

struct 
    [[nodiscard]] 
    Result {};

struct 
    Result2 {};

class Test 
{
public:
    Result const doSomething() { return Result {}; }

    [[nodiscard]]
    Result2 const doSomethingElse() { return Result2 {}; }
}; 

int main()
{
    Test t {};
    t.doSomething();     // Warning: discarded-value expression
    t.doSomethingElse(); // Warning: discarded-value expression
}

This code will compile but, if the encouragment has been taken seriously by compiler implementers, a warning will be emitted (e.g. -Wunused-result on GCC ).

Always [[nodiscard]]!

[[nodiscard]] at first sight seems to be a good thing.
In fact so good, that the idea comes up to just use it everywhere, to make sure the compiler will tell you all your missed opportunities to use the full potential of your design and API.

Reading https://www.bfilipek.com/2017/11/nodiscard.html, for example, introduces applications of [[nodiscard]] for error handling (status codes) and an elegant way to highlight the use of functions whose usage is discouraged.

The interesting thing is, that these applications of [[nodiscard]] can be used to track down design smells in your code.

An example:

template <typename T>
bool SAFE_DELETE(T *aPointer)
{
    if(nullptr == aPointer)
    {
        return false;
    } 

    delete aPointer;
    return true;
}

// @ 90% of all occurrences
// ...
SAFE_DELETE(pInstance); // discarded-value expression
// ...

I have seen the above code in a customer’s codebase and I realized two issues here:

1. Catching deletion of a nullptr
2. Returning a bool-result.

Deleting a nullptr is well defined since C++14 and as such the whole if-clause is unnecessary.
But if it was necessary, there’s no point in returning a boolean value, if the return value is ignored in the code base anyway.

If the pointer is a nullptr, make it a no-op.

Using [[nodiscard]] everywhere, also in the above function, would have signalled, that a return value is ignored and developers might have detected the bad quality code above.

template <typename T>
[[nodiscard]]
bool SAFE_DELETE(T *aPointer)
{
    if(nullptr == aPointer)
    {
        return false;
    } 

    delete aPointer;
    return true;
}

// @ 90% of all occurrences
// ...
SAFE_DELETE(pInstance); // Warning: discarded-value expression
// ...

But it’s just a warning…

… and they are usually ignored as low priority anyway!

Although the quality implications of not having cleaning-cycles or being ignorant towards warnings in development are worth a discussion, let’s move on and find a solution for this:

Turn on -Werror=unused-result or -Werror
(MSVC: /We#### and /WX)

This will have the compilation fail for any discarded value in your code base ( and the latter: for any other ignored warning… ).

But…

… what if I really, really don’t need the value?

[[maybe_unused]]
As an attribute will suppress warnings on unused entities (also for e.g. unused functions or arguments).

[[nodiscard]]
bool someOperation() { ... }

bool result = someOperation();
// No further access to result 
//     -> discarded-value expression 
//     -> Warning

// Solution:
[[maybe_unused]] bool result = someOperation(); // OK.

But what if there’s no result-variable specified for the call, i.e. someOperation(); ?

[[maybe_unused]] is of no help here, as[[maybe_unused]] someOperation(); will have the compilation fail.

Meet: EXPLICIT_DISCARD(expression)

My solution to this issue is a preprocessor macro called EXPLICIT_DISCARD(expression), defined as:

#define EXPLICIT_DISCARD(x) { [[maybe_unused]] auto __v = x; }

// Use w/ the last snippet's code:
EXPLICIT_DISCARD(someOperation());

This is useful as it permits the always [[nodiscard]] style to be employed, while handling different variations of discarded-value expressions for warning/error suppression!

But be aware…

… that EXPLICIT_DISCARD is more of a HACK, than a solution!
I you have an extraordinary amount of unused return values, you should consider the necessity of the return value at all and refactor your design!

Conclusion

One tool to increase code quality during coding is employing a simple rule: Always [[nodiscard]]
This way, the compiler will cry like a baby whose lollipop has been stolen each time a return value would be discarded.

Using -Werror= or -Werror will completely enforce the requirement!

And finally, if there ever is a case of necessity to discard a value as it is not needed, be explicit and use [[maybe_unused]] or the EXPLICIT_DISCARD(expression) macro.

Prospect to C++20

C++20 will enhance [[nodiscard]] even more.
It will be possible to provide a textual hint to declarations explaining why the value may not be discarded.

enum class
[[nodiscard("Error codes must be handled!")]]
EError 
    : uint8_t
{
    OK             = 0,
    DriverNotFound,
    MaxEnum        = 255
}

EError somethingCritical() { ... }

int main()
{
    somethingCritical(); // Ohoh... but with context-info!
}

Unfortunately, [[maybe_unused]] won’t have such an additional text to explain, why an explicit discard is employed in the code base!

(Could be my first C++ Standard proposal :P)