1293117 - Fix various NS_IMETHOD/NS_IMETHODIMP/NS_METHOD misuses

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

Our use of NS_METHOD, NS_IMETHOD and NS_IMETHODIMP is a mess. It's supposed to
be like this:

- In-class declarations:    NS_IMETHOD    if virtual, NS_METHOD otherwise
- In-class definitions:     NS_IMETHOD    if virtual, NS_METHOD otherwise
- Out-of-class definitions: NS_IMETHODIMP if virtual, NS_METHOD otherwise

The meanings of these macros are as follows.

- NS_IMETHOD basically means |virtual nsresult|.
- NS_METHOD and NS_IMETHODIMP both basically mean |nsresult|.

(The "basically" above is because on Win32 all three also add "__stdcall", but
that's not the part I'm concerned with now.)

The NS_IMETHOD/NS_IMETHODIMP split is necessary because |virtual| isn't allowed
on out-of-class definitions.

The three macros are often misused.

- NS_METHOD and NS_IMETHODIMP can be mixed up anywhere (without causing
  problems) because they have the same underlying meaning.

- Because |virtual| is implicit when overriding a virtual function, NS_METHOD
  and NS_IMETHODIMP can be used erronously (without causing problems) instead
  of NS_IMETHOD in that case.

Fixing all the misuses is hard, but there are some easy patterns. For example:

- |virtual NS_IMETHODIMP| / |virtual NS_METHOD| should just be |NS_IMETHOD|.

- Any in-class declaration/definition that has |override| or |final| must be
  virtual, and so should use NS_IMETHOD.

I want to fix this because it's annoying me and getting in my way while doing
bug 1292440.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add comment explaining how NS_IMETHOD, NS_IMETHODIMP and NS_METHOD should be used

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove unnecessary comments on some NS_IMETHODIMP instances

This patch replaces numerous |/* virtual */ NS_IMETHODIMP| occurrences with
|NS_IMETHODIMP|, because |NS_IMETHODIMP| always implies |virtual|.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove some unnecessary |virtual| annotations

This patch changes |virtual NS_IMETHODIMP| occurrences to |NS_IMETHOD|, which
is equivalent and the more standard way of marking in-class virtual XPCOM
methods.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Change many NS_IMETHODIMP occurrences to NS_IMETHOD

This patch makes the following changes on many in-class methods.

- NS_IMETHODIMP F() override;      --> NS_IMETHOD F() override;
- NS_IMETHODIMP F() override {...} --> NS_IMETHOD F() override {...}
- NS_IMETHODIMP F() final;         --> NS_IMETHOD F() final;
- NS_IMETHODIMP F() final {...}    --> NS_IMETHOD F() final {...}

Using NS_IMETHOD is the preferred way of marking in-class virtual methods.
Although these transformations add an explicit |virtual|, they are safe --
there's an implicit |virtual| anyway because |override| and |final| only work
with virtual methods.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Change many NS_METHOD occurrences to NS_IMETHOD

This patch makes the following changes on many in-class methods.

- NS_METHOD F() override;      --> NS_IMETHOD F() override;
- NS_METHOD F() override {...} --> NS_IMETHOD F() override {...}
- NS_METHOD F() final;         --> NS_IMETHOD F() final;
- NS_METHOD F() final {...}    --> NS_IMETHOD F() final {...}

Using NS_IMETHOD is the preferred way of marking in-class virtual methods.
Although these transformations add an explicit |virtual|, they are safe --
there's an implicit |virtual| anyway because |override| and |final| only work
with virtual methods.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Fix a comment and return type in XPCComponents.cpp

The comment is wrong. NS_METHOD is the appropriate thing to use here.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add comment explaining how NS_IMETHOD, NS_IMETHODIMP and NS_METHOD should be used

New descriptions based on my updated understanding. (NS_METHOD and NS_CALLBACK
don't seem to serve any useful purpose and I'm wondering if they can be
removed.)

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove two occurrences of |static NS_IMETHODIMP|

Because it's a combination that doesn't make sense.

- ReadFuncBinaryString becomes NS_METHOD, because it's passed to ReadSegments,
  which requires a function matching NS_CALLBACK.

- IccContactListToMozContactList just changes to a vanilla nsresult return
  type, because it doesn't need __stdcall on Windows.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8779165 [details] [diff] [review]
(part 6) - Fix a comment and return type in XPCComponents.cpp

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

It might be worth noting that this function is passed to an NS_CALLBACK entity, and NS_METHOD gets us the correct __stdcall modifier on Windows.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8779274 [details] [diff] [review]
(part 7) - Remove two occurrences of |static NS_IMETHODIMP|

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

WFM.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8779105 [details] [diff] [review]
(part 4) - Change many NS_IMETHODIMP occurrences to NS_IMETHOD

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

WFM.  People will probably continue to use NS_IMETHODIMP for definitions inside the class, but whaddya gonna do?

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8779166 [details] [diff] [review]
(part 1) - Add comment explaining how NS_IMETHOD, NS_IMETHODIMP and NS_METHOD should be used

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

r=me assuming we can agree on phrasing below.

::: xpcom/base/nscore.h
@@ +173,5 @@
>   * Generic API modifiers which return the standard XPCOM nsresult type
> + *
> + * - NS_IMETHOD: use for in-class declarations and definitions.
> + * - NS_IMETHODIMP: use for out-of-class definitions.
> + * - NS_METHOD: use only in conjunction with NS_CALLBACK.

I don't think this is appropriate; it gets used in the generated code for bug 1293870, for instance, even though there's no NS_CALLBACK stuff in sight.

How about "used for non-virtual methods that return nsresult"?

Comment 13

[Nicholas Nethercote [inactive]]

> > + * - NS_METHOD: use only in conjunction with NS_CALLBACK.
> 
> I don't think this is appropriate; it gets used in the generated code for
> bug 1293870, for instance, even though there's no NS_CALLBACK stuff in sight.

But it's not really necessary in that case, right?

> How about "used for non-virtual methods that return nsresult"?

That's not quite right, though; you might as well just use vanilla nsresult in that case.

How about just "usually used in conjunction with NS_CALLBACK"? I'm hoping to get rid of NS_METHOD anyway...

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/581bf8b4f433e44150ae7d11fa033064608e443c
Bug 1293117 (part 1) - Add comment explaining how NS_IMETHOD, NS_IMETHODIMP and NS_METHOD should be used. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2764e29be7dce8539f6194f11d479345577db2c5
Bug 1293117 (part 2) - Remove unnecessary comments on some NS_IMETHODIMP instances. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4db4249540773fabd59acb0e3fe883c95d3213f4
Bug 1293117 (part 3) - Remove some unnecessary |virtual| annotations. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/babe907f527681f351d62d689230d23e55d6a729
Bug 1293117 (part 4) - Change many NS_IMETHODIMP occurrences to NS_IMETHOD. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2605dceca7b26e11f3a2c546cf5aaea7d0fb9385
Bug 1293117 (part 5) - Change many NS_METHOD occurrences to NS_IMETHOD. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/05133363f4edf4633ce9ee0dff1ce31f56908c9d
Bug 1293117 (part 6) - Fix a comment and return type in XPCComponents.cpp. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/32e07e32891d070cdba97e2872cd30265278c0bf
Bug 1293117 (part 7) - Remove two occurrences of |static NS_IMETHODIMP|. r=froydnj.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/581bf8b4f433
https://hg.mozilla.org/mozilla-central/rev/2764e29be7dc
https://hg.mozilla.org/mozilla-central/rev/4db424954077
https://hg.mozilla.org/mozilla-central/rev/babe907f5276
https://hg.mozilla.org/mozilla-central/rev/2605dceca7b2
https://hg.mozilla.org/mozilla-central/rev/05133363f4ed
https://hg.mozilla.org/mozilla-central/rev/32e07e32891d