Closed
Bug 1293117
Opened 7 years ago
Closed 7 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•7 years ago
|
||
Attachment #8779102 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•7 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•7 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•7 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•7 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•7 years ago
|
||
The comment is wrong. NS_METHOD is the appropriate thing to use here.
Attachment #8779165 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 7•7 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•7 years ago
|
Attachment #8779102 -
Attachment is obsolete: true
Attachment #8779102 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 8•7 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•7 years ago
|
Attachment #8779103 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•7 years ago
|
Attachment #8779104 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 9•7 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•7 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•7 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•7 years ago
|
Attachment #8779107 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 12•7 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•7 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•7 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•7 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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•