Closed Bug 1293117 Opened 9 years ago Closed 9 years ago

Fix various NS_IMETHOD/NS_IMETHODIMP/NS_METHOD misuses

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(7 files, 1 obsolete file)

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: nobody → n.nethercote
Status: NEW → ASSIGNED
This patch replaces numerous |/* virtual */ NS_IMETHODIMP| occurrences with |NS_IMETHODIMP|, because |NS_IMETHODIMP| always implies |virtual|.
Attachment #8779103 - Flags: review?(nfroyd)
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)
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)
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)
The comment is wrong. NS_METHOD is the appropriate thing to use here.
Attachment #8779165 - Flags: review?(nfroyd)
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)
Attachment #8779102 - Attachment is obsolete: true
Attachment #8779102 - Flags: review?(nfroyd)
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)
Attachment #8779103 - Flags: review?(nfroyd) → review+
Attachment #8779104 - Flags: review?(nfroyd) → review+
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 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 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+
Attachment #8779107 - Flags: review?(nfroyd) → review+
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+
> > + * - 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...
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: