Rename MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
MOZ_WARN_UNUSED_RESULT is offputting long. I'm about to add a lot of
these annotations to SpiderMonkey. A shorter synonym would be nice.
Assignee

Comment 1

3 years ago
I'm not especially wedded to MOZ_CHECK. Something else equally short would
likely be equally good.
Attachment #8745197 - Flags: review?(nfroyd)
Assignee

Updated

3 years ago
Blocks: 1267551
In my opinion, MOZ_CHECK is confusingly generic. What are you checking? 

Maybe MOZ_USE_RESULT?
Also if it is good enough to use sometimes, we should probably use it all the time, rather than having multiple names for it sitting around.
Comment on attachment 8745197 [details] [diff] [review]
Add MOZ_CHECK as a synonym for MOZ_WARN_UNUSED_RESULT

Review of attachment 8745197 [details] [diff] [review]:
-----------------------------------------------------------------

I'm in agreement with mccr8 here; if we're going to use something shorter, we should use it all the time.  And MOZ_CHECK doesn't really communicate much (check the return value?  check that the provided argument has size less than void*?).  I like fitzgen's suggestion; it's a bit more evocative than MOZ_CHECK.

Length comparisons:

MOZ_CHECK
MOZ_MUST_USE
MOZ_MUST_USE_RV ("return value")
MOZ_WARN_UNUSED_RESULT

Though if we're going to get into length comparisons, doing this as a single-argument macro takes more characters (assuming you put the return type and the function name all on one line). ;)
Attachment #8745197 - Flags: review?(nfroyd)
Waldo, do you have a favorite color for the bikeshed here?
Flags: needinfo?(jwalden+bmo)
I got `MOZ_MUST_USE` from Rust's `#[must_use]` attribute, FWIW.
MOZ_MUST_USE seems cromulent.  And yes, no way are we doing the synonym thing.  :-)
Flags: needinfo?(jwalden+bmo)
The name MOZ_MUSE_USE is already used for a type attribute (bug 1180993).

https://mxr.mozilla.org/mozilla-central/ident?i=MOZ_MUST_USE
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> MOZ_MUST_USE seems cromulent.  And yes, no way are we doing the synonym
> thing.  :-)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #7)
> I got `MOZ_MUST_USE` from Rust's `#[must_use]` attribute, FWIW.

Let the oxidation of Gecko continue, then.

(In reply to Chris Peterson [:cpeterson] from comment #9)
> The name MOZ_MUSE_USE is already used for a type attribute (bug 1180993).
> 
> https://mxr.mozilla.org/mozilla-central/ident?i=MOZ_MUST_USE

r+ on changing that to something else.  MOZ_MUST_USE_AS_RETURN_VALUE?
Assignee

Comment 11

3 years ago
This will allow MOZ_MUST_USE to be used for a different and more common case.
Attachment #8745665 - Flags: review?(ehsan)

Comment 12

3 years ago
Both MOZ_MUST_USE and MOZ_MUST_USE_TYPE are essentially trying to achieve the same goal, but in slightly different ways.  Have you considered the alternative of making MOZ_MUST_USE expand to both of these attributes so that we'd use the same identifier for both cases?  The only issue is that gcc would warn if warn_unused_result is used on a type, which can be fixed with building with -Wno-attributes.

What do you think, Nick?
Flags: needinfo?(n.nethercote)
Assignee

Comment 13

3 years ago
It's an annotation that is used a lot, and should be used even more, so a
shorter name is better.
Attachment #8745843 - Flags: review?(nfroyd)
Assignee

Updated

3 years ago
Attachment #8745197 - Attachment is obsolete: true
Flags: needinfo?(n.nethercote)
Assignee

Comment 14

3 years ago
(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #12)
> Both MOZ_MUST_USE and MOZ_MUST_USE_TYPE are essentially trying to achieve
> the same goal, but in slightly different ways.  Have you considered the
> alternative of making MOZ_MUST_USE expand to both of these attributes so
> that we'd use the same identifier for both cases?  The only issue is that
> gcc would warn if warn_unused_result is used on a type, which can be fixed
> with building with -Wno-attributes.
> 
> What do you think, Nick?

I understand that they have similar goals, but aren't they different mechanisms that can't be used in tandem?

AIUI, MOZ_MUST_USE_TYPE applies to a *type*, and means "any time this type is used as a return value, implicitly add MOZ_MUST_USE to it". Is using MOZ_MUST_USE_TYPE on a function return type even valid?
Flags: needinfo?(ehsan)

Comment 15

3 years ago
(In reply to Nicholas Nethercote [:njn] from comment #14)
> (In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment
> #12)
> > Both MOZ_MUST_USE and MOZ_MUST_USE_TYPE are essentially trying to achieve
> > the same goal, but in slightly different ways.  Have you considered the
> > alternative of making MOZ_MUST_USE expand to both of these attributes so
> > that we'd use the same identifier for both cases?  The only issue is that
> > gcc would warn if warn_unused_result is used on a type, which can be fixed
> > with building with -Wno-attributes.
> > 
> > What do you think, Nick?
> 
> I understand that they have similar goals, but aren't they different
> mechanisms that can't be used in tandem?

Well as things are right now, they are different unrelated mechanisms, but I'm proposing to change that.

> AIUI, MOZ_MUST_USE_TYPE applies to a *type*, and means "any time this type
> is used as a return value, implicitly add MOZ_MUST_USE to it".

Yes.

> Is using
> MOZ_MUST_USE_TYPE on a function return type even valid?

Well, it is valid in that the clang-plugin will just ignore it.

The way that I think about my suggestion is like this: If you put MOZ_MUST_USE on a type, you're requiring that the return values of that type to always be used.  If you put it on a function, you're requiring that the return values of just that one function to always be used.

(Note that I'm not advocating for this super-strongly, so if you disagree your current patch is fine too, I just think if we can make people have to remember one fewer thing it's better!)
Flags: needinfo?(ehsan)
Assignee

Comment 16

3 years ago
Given that MOZ_MUST_USE_TYPE is rare, and MOZ_MUST_USE is common (and will become more so), I don't conflating them is more likely to cause problems than help.

Comment 17

3 years ago
Comment on attachment 8745665 [details] [diff] [review]
(part 1) - Rename MOZ_MUST_USE as MOZ_MUST_USE_TYPE

Review of attachment 8745665 [details] [diff] [review]:
-----------------------------------------------------------------

OK, fair enough!
Attachment #8745665 - Flags: review?(ehsan) → review+
Comment on attachment 8745843 [details] [diff] [review]
(part 2) - Rename MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE

Review of attachment 8745843 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming this was done s/MOZ_WARN_UNUSED_RESULT/MOZ_MUST_USE/ or similar, rs=me.
Attachment #8745843 - Flags: review?(nfroyd) → review+
Assignee

Comment 19

3 years ago
> Assuming this was done s/MOZ_WARN_UNUSED_RESULT/MOZ_MUST_USE/ or similar,
> rs=me.

I did it semi-automatically, using Vim macros. If I'd done it automatically I wouldn't have fixed up indentation in multi-line function signatures.
Assignee

Updated

3 years ago
Summary: Add MOZ_CHECK as a synonym for MOZ_WARN_UNUSED_RESULT → Rename MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4fe5cc4663a3
https://hg.mozilla.org/mozilla-central/rev/3d67e45f994a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.