Remove NS_OVERRIDE and replace all uses of it with MOZ_OVERRIDE

RESOLVED FIXED in mozilla17

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Waldo, Assigned: jcranmer)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

6 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.

Updated

6 years ago
Depends on: 702437
(Assignee)

Comment 2

5 years ago
Created attachment 641861 [details] [diff] [review]
Replace all uses in mozilla-central

This kills all uses of NS_OVERRIDE and deletes the macro to make sure no one else tries to use it again.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #641861 - Flags: review?(benjamin)
(Assignee)

Comment 3

5 years ago
Created attachment 642018 [details] [diff] [review]
Version that passes tinderbox

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

5 years ago
Attachment #642018 - Flags: review?(benjamin) → review?(ehsan)
Duplicate of this bug: 727279
Attachment #642018 - Flags: review?(ehsan) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/950048a58302
https://hg.mozilla.org/mozilla-central/rev/950048a58302
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
There's still xpcom/analysis/override.js that refers to NS_OVERRIDE.
(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!
Depends on: 817387
You need to log in before you can comment on or make changes to this bug.