C++ PSA: Use [[nodiscard]] instead of MOZ_MUST_USE

[Chris Peterson]

Now that Firefox is compiled as C++17 (bug 1560664), you can use C++17's
[[nodiscard]] attribute [1] instead of the MOZ_MUST_USE macro (defined
using clang and gcc's non-standard __attribute__((warn_unused_result))).

I have been slowly replacing MOZ_MUST_USE with [[nodiscard]] in my free
time and hope to eventually remove the MOZ_MUST_USE definition itself.
That is meta bug 1571631.

In the meantime, please:

1. Avoid adding more uses of MOZ_MUST_USE. Use [[nodiscard]].

2. Consider making more functions use [[nodiscard]] when writing or
reviewing new code. Functions that return errors as nsresult or bool are
probably good candidates for [[nodiscard]].

(I looked at adding [[nodiscard]] to the nsresult type definition, but
the results were too noisy.)

One caveat: the [[nodiscard]] attribute must precede all of a function
declaration's declaration specifiers (like static, extern, inline, or
virtual). The __attribute__((warn_unused_result)) attribute (and thus
MOZ_MUST_USE) does not have this order restriction.

- static inline MOZ_MUST_USE nsresult SomeFunction();
+ [[nodiscard]] static inline nsresult SomeFunction();

Once __attribute__((warn_unused_result)) has been replaced with
[[nodiscard]], we can also remove mozilla::Unused, replacing `Unused <<`
with a more idiomatic `(void)` cast (bug 1628542).

[[nodiscard]] can also be applied to types, so we may be able to replace
our custom MOZ_MUST_USE_TYPE clang plugin with [[nodiscard]].

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

[Andi-Bogdan Postelnicu]

Hello Chris,

Thank you so much for doing this. From a static-analysis perspective this helps us greatly avoid possible false positives and enforcing the rule when an actual result return from a function call shouldn’t be discarded.

Andi

> On 22 Dec 2020, at 02:25, Chris Peterson <cpet...@mozilla.com> wrote:
>
> Now that Firefox is compiled as C++17 (bug 1560664), you can use C++17's [[nodiscard]] attribute [1] instead of the MOZ_MUST_USE macro (defined using clang and gcc's non-standard __attribute__((warn_unused_result))).

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Chris Peterson]

Bug 1571631 has landed. The `MOZ_MUST_USE` macro has been removed and
all uses have been replaced with C++17's `[[nodiscard]]` function
attribute. Use [[nodiscard]] instead of MOZ_MUST_USE.

One caveat: unlike MOZ_MUST_USE, [[nodiscard]] must precede all of a

function declaration's declaration specifiers (like static, extern,

inline, or virtual):

- static inline MOZ_MUST_USE nsresult SomeFunction();
+ [[nodiscard]] static inline nsresult SomeFunction();

Next steps:

1. Replace MOZ_MUST_USE_TYPE type declarations with [[nodiscard]]. Bug
1628542

2. Consider removing `mozilla::Unused`, replacing over 3000 uses of
`Unused << SomeFunction()` with a more idiomatic `(void) SomeFunction()`
cast. mozilla::Unused was a workaround for the fact that gcc didn't
allow MOZ_MUST_USE (`__attribute__((warn_unused_result))`) warnings to
be suppressed with a void cast. Because this change would touch so many
files, it's probably not worth the trouble. Bug 1628542