1571631 - (nodiscard) Replace MOZ_MUST_USE macro with C++17 attribute [[nodiscard]]

Status: RESOLVED FIXED

(Core :: MFBT, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

After we build as C++17 by default, we can replace MOZ_MUST_USE with C++17 [[nodiscard]].

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

Comment 2

[Chris Peterson [:cpeterson]]

The MOZ_MUST_USE macro is defined as clang's and gcc's nonstandard __attribute__((warn_unused_result)). After we compile as C++17 by default (bug 1560664), we can replace MOZ_MUST_USE with C++17's standard [[nodiscard]] attribute.

https://searchfox.org/mozilla-central/rev/35873cfc312a6285f54aa5e4ec2d4ab911157522/mfbt/Attributes.h#392

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

TBD whether the MOZ_ALLOCATOR macro can also be switched to [[nodiscard]]:

https://searchfox.org/mozilla-central/rev/35873cfc312a6285f54aa5e4ec2d4ab911157522/mfbt/Attributes.h#355

However, these things are never as straightforward as they seem. clang allows __attribute__((warn_unused_result)) in some places that [[nodiscard]] is not. Perhaps those __attribute__((warn_unused_result)) uses can be reordered to allow [[nodiscard]] or they should remain.

Comment 3

[Chris Peterson [:cpeterson]]

Unfortunately, this change will be more complicated than a global find and replace of MOZ_MUST_USE for [[nodiscard]]. The [[nodiscard]] attribute must precede a function declaration's declaration specifiers (like static, extern, inline, or virtual). The __attribute__((warn_unused_result)) attribute did not have this order restriction, so we have hundreds of cases like:

extern MOZ_MUST_USE bool foo();
inline MOZ_MUST_USE bool foo();
static MOZ_MUST_USE bool foo();
static inline MOZ_MUST_USE bool foo();
virtual MOZ_MUST_USE bool foo();

that will need to be rewritten as:

[[nodisard]] extern bool foo();
[[nodisard]] inline bool foo();
[[nodisard]] static bool foo();
[[nodisard]] static inline bool foo();
[[nodisard]] virtual bool foo();

Comment 4

[Karl Tomlinson (:karlt)]

The behavior of nodiscard differs from warn_unused_result in gcc, where a void cast does not suppress warn_unused_result warnings. The difference does not exist in clang.

Moving to nodiscard will make mozilla::Unused obsolete.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Karl Tomlinson (:karlt) from comment #4)

The behavior of nodiscard differs from warn_unused_result in gcc, where a void cast does not suppress warn_unused_result warnings. The difference does not exist in clang.

Moving to nodiscard will make mozilla::Unused obsolete.

Can uses of Unused << then be globally replaced with (void)? Like:

- Unused << SendShowEvent(aData, aFromUser);
+ (void) SendShowEvent(aData, aFromUser);

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Chris Peterson [:cpeterson] from comment #5)

Can uses of Unused << then be globally replaced with (void)? Like:

- Unused << SendShowEvent(aData, aFromUser);
+ (void) SendShowEvent(aData, aFromUser);

Yes, except that clang-format prefers no space after (void).

I'm not a style peer, but at least one peer would appreciate that.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Bug 1571631 - Replace MOZ_MUST_USE with [[nodiscard]] in layout/style/SharedStyleSheetCache.h. r?#layout-reviewers

MOZ_MUST_USE was replaced with [[nodiscard]] in layout in bug 1625855, but some new uses of MOZ_MUST_USE instead of [[nodiscard]] have crept back in.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: Bug 1571631 - Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/commandlines/nsCommandLine.h. r?mossop

Depends on D108342

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: Bug 1571631 - Replace MOZ_MUST_USE with [[nodiscard]] in js/src. r?jandem

MOZ_MUST_USE was replaced with [[nodiscard]] in js/rc in bug 1684092, but some new uses of MOZ_MUST_USE instead of [[nodiscard]] have crept back in.

Depends on D108343

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: Bug 1571631 - Replace MOZ_MUST_USE with [[nodiscard]] in mfbt. r?jwalden

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's attribute((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Depends on D108344

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: Bug 1571631 - Remove MOZ_MUST_USE definition. r?jwalden

Depends on D108345

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/362aed67c493
Replace MOZ_MUST_USE with [[nodiscard]] in layout/style/SharedStyleSheetCache.h. r=emilio
https://hg.mozilla.org/integration/autoland/rev/08fc8b587c05
Replace MOZ_MUST_USE with [[nodiscard]] in toolkit/components/commandlines/nsCommandLine.h. r=mossop
https://hg.mozilla.org/integration/autoland/rev/ad427a2f4a2f
Replace MOZ_MUST_USE with [[nodiscard]] in js/src. r=jandem

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/362aed67c493
https://hg.mozilla.org/mozilla-central/rev/08fc8b587c05
https://hg.mozilla.org/mozilla-central/rev/ad427a2f4a2f

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32c92dfb8fe7
Replace MOZ_MUST_USE with [[nodiscard]] in mfbt. r=sg
https://hg.mozilla.org/integration/autoland/rev/f9a8f6b334ef
Remove MOZ_MUST_USE definition. r=sg

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/32c92dfb8fe7
https://hg.mozilla.org/mozilla-central/rev/f9a8f6b334ef