Closed
Bug 702877
Opened 13 years ago
Closed 13 years ago
Remove NS_OVERRIDE and replace all uses of it with MOZ_OVERRIDE
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Waldo, Assigned: jcranmer)
References
Details
Attachments
(1 file, 1 obsolete file)
166.55 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
NS_OVERRIDE and MOZ_OVERRIDE both indicate the same thing: that the virtual function member being annotated must override a virtual function member on a base class. MOZ_OVERRIDE abstracts away new C++11 goodness and actually has teeth in some recent compilers. NS_OVERRIDE is a user attribute and has no teeth except in static analysis builds, and I don't even know what the status of those is any more.
Probably static analysis can look for MOZ_OVERRIDE in new enough gcc/clang compilers (4.7+ and 3.0+, respectively, but note gcc 4.7 isn't released yet), so NS_OVERRIDE and all uses of it should be removed and replaced with MOZ_OVERRIDE.
Assignee | ||
Comment 1•13 years ago
|
||
Some notes here:
1. NS_OVERRIDE is generally done before the method declaration, whereas the override keyword would have to be after the method declaration. So all uses of NS_OVERRIDE need to be moved around.
2. It should be possible to alias MOZ_OVERRIDE to the __attribute__((user("NS_override"))) version in sufficiently old (i.e., gcc 4.6-or-less) builds.
Assignee | ||
Comment 2•13 years ago
|
||
This kills all uses of NS_OVERRIDE and deletes the macro to make sure no one else tries to use it again.
Assignee | ||
Comment 3•13 years ago
|
||
Silly me, why did I assume it worked on all platforms just because it worked on my computer?
Attachment #641861 -
Attachment is obsolete: true
Attachment #641861 -
Flags: review?(benjamin)
Attachment #642018 -
Flags: review?(benjamin)
Assignee | ||
Updated•13 years ago
|
Attachment #642018 -
Flags: review?(benjamin) → review?(ehsan)
Updated•13 years ago
|
Attachment #642018 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 5•13 years ago
|
||
Comment 6•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 7•13 years ago
|
||
There's still xpcom/analysis/override.js that refers to NS_OVERRIDE.
Comment 8•13 years ago
|
||
(In reply to comment #7)
> There's still xpcom/analysis/override.js that refers to NS_OVERRIDE.
Good catch. I think that entire script should be removed. Can you please file a bug on that? Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•