Closed
Bug 1293117
Opened 9 years ago
Closed 9 years ago
Fix various NS_IMETHOD/NS_IMETHODIMP/NS_METHOD misuses
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(7 files, 1 obsolete file)
20.08 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
4.07 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
87.98 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
17.02 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.98 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•9 years ago
|
||
Attachment #8779102 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Updated•9 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•9 years ago
|
||
This patch replaces numerous |/* virtual */ NS_IMETHODIMP| occurrences with
|NS_IMETHODIMP|, because |NS_IMETHODIMP| always implies |virtual|.
Attachment #8779103 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 3•9 years ago
|
||
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.
Attachment #8779104 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 4•9 years ago
|
||
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.
Attachment #8779105 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 5•9 years ago
|
||
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.
Attachment #8779107 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 6•9 years ago
|
||
The comment is wrong. NS_METHOD is the appropriate thing to use here.
Attachment #8779165 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 7•9 years ago
|
||
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.)
Attachment #8779166 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8779102 -
Attachment is obsolete: true
Attachment #8779102 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 8•9 years ago
|
||
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.
Attachment #8779274 -
Flags: review?(nfroyd)
![]() |
||
Updated•9 years ago
|
Attachment #8779103 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•9 years ago
|
Attachment #8779104 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 9•9 years ago
|
||
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.
Attachment #8779165 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 10•9 years ago
|
||
Comment on attachment 8779274 [details] [diff] [review]
(part 7) - Remove two occurrences of |static NS_IMETHODIMP|
Review of attachment 8779274 [details] [diff] [review]:
-----------------------------------------------------------------
WFM.
Attachment #8779274 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 11•9 years ago
|
||
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?
Attachment #8779105 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•9 years ago
|
Attachment #8779107 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 12•9 years ago
|
||
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"?
Attachment #8779166 -
Flags: review?(nfroyd) → review+
![]() |
Assignee | |
Comment 13•9 years ago
|
||
> > + * - 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...
![]() |
Assignee | |
Comment 14•9 years ago
|
||
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•9 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•