1267550 - Rename MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add MOZ_CHECK as a synonym for MOZ_WARN_UNUSED_RESULT

I'm not especially wedded to MOZ_CHECK. Something else equally short would
likely be equally good.

Comment 2

[Andrew McCreight [:mccr8]]

In my opinion, MOZ_CHECK is confusingly generic. What are you checking? 

Maybe MOZ_USE_RESULT?

Comment 3

[Andrew McCreight [:mccr8]]

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 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

MOZ_MUST_USE ?

Comment 5

[Nathan Froyd [:froydnj]]

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). ;)

Comment 6

[Nathan Froyd [:froydnj]]

Waldo, do you have a favorite color for the bikeshed here?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I got `MOZ_MUST_USE` from Rust's `#[must_use]` attribute, FWIW.

Comment 8

[Jeff Walden [:Waldo]]

MOZ_MUST_USE seems cromulent.  And yes, no way are we doing the synonym thing.  :-)

Comment 9

[Chris Peterson [:cpeterson]]

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

Comment 10

[Nathan Froyd [:froydnj]]

(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?

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Rename MOZ_MUST_USE as MOZ_MUST_USE_TYPE

This will allow MOZ_MUST_USE to be used for a different and more common case.

Comment 12

[(no longer active)]

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?

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Rename MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE

It's an annotation that is used a lot, and should be used even more, so a
shorter name is better.

Comment 14

[Nicholas Nethercote [inactive]]

(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?

Comment 15

[(no longer active)]

(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!)

Comment 16

[Nicholas Nethercote [inactive]]

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

[(no longer active)]

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!

Comment 18

[Nathan Froyd [:froydnj]]

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.

Comment 19

[Nicholas Nethercote [inactive]]

> 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.

Comment 20

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe5cc4663a37c77239fd9f3344d4816fa7c9ce6
Bug 1267550 (part 1) - Rename MOZ_MUST_USE as MOZ_MUST_USE_TYPE. r=ehsan.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d67e45f994a3ae1ae43479c32c2ed63bbf1a7c8
Bug 1267550 (part 2) - Rename MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE. r=froydnj.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fe5cc4663a3
https://hg.mozilla.org/mozilla-central/rev/3d67e45f994a