Rename NS_MUST_OVERRIDE to NS_MUST_BE_OVERRIDDEN, and remove NS_OVERRIDE in favor of MOZ_OVERRIDE

RESOLVED DUPLICATE of bug 702877

Status

()

RESOLVED DUPLICATE of bug 702877
7 years ago
7 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Our override macros are confusing.

The lesser of the two evils is NS_OVERRIDE.  NS_OVERRIDE enforces the same contract as MOZ_OVERRIDE, which resolves to C++11's |override| keyword, which means "this function must override something."  Cool.

But while MOZ_OVERRIDE has some compiler support, NS_OVERRIDE interfaces only with static-analysis, which we don't run.  Therefore, I propose we s/NS_OVERRIDE/MOZ_OVERRIDE/g.


It's not at all clear from the name what NS_MUST_OVERRIDE means, since there's an implicit "must" in NS_OVERRIDE's contract.  In fact, NS_MUST_OVERRIDE means "this function must *be overridden*":

> * NS_MUST_OVERRIDE:
> *   a method which every immediate subclass of this class must
> *   override.  A subclass override can itself be NS_MUST_OVERRIDE, in
> *   which case its own subclasses must override the method as well.

NS_MUST_BE_OVERRIDDEN is a much better name for this than NS_MUST_OVERRIDE.
(Assignee)

Updated

7 years ago
Assignee: nobody → justin.lebar+bug
Sigh, it looks like you can't specify |override| on an inline virtual function definition:

class Foo : public Bar {
  public virtual bar() override { ... } // <-- error
};
Oh,  nevermind.  It's only an error if you can't spell...
This has a few false-positives, which are fixed in a later patch.

$ git ls-files | xargs grep -l 'NS_OVERRIDE.*;' | xargs -n1 sed -i -e 's/NS_OVERRIDE \(.*\);/\1 MOZ_OVERRIDE;/'
Attachment #597956 - Flags: review?(benjamin)
NS_MUST_BE_OVERRIDDEN doesn't make sense on a non-virtual function or on functions in a MOZ_FINAL class.
Attachment #597961 - Flags: review?(n.nethercote)
(Assignee)

Updated

7 years ago
Attachment #597954 - Flags: review?(benjamin)
(Assignee)

Updated

7 years ago
Attachment #597960 - Flags: review? → review?(benjamin)
These changes are almost entirely mechanical, so I tried to split up the patches into their component parts, explaining the steps I took.  I'll qfold all of part 1 and part 2 before pushing.

I've tested building with clang (which, I tested, has a non-empty MOZ_OVERRIDE definition).

I didn't find anything incorrectly marked with NS_OVERRIDE, but I did find three functions incorrectly marked with NS_MUST_OVERRIDE, which makes me think this is at least a slightly virtuous change.
Attachment #597961 - Flags: review?(n.nethercote) → review+

Updated

7 years ago
Attachment #597954 - Flags: review?(benjamin) → review+

Updated

7 years ago
Attachment #597956 - Flags: review?(benjamin) → review+
Comment on attachment 597957 [details] [diff] [review]
Part 1c, v1: Replace remaining NS_OVERRIDEs, and fix part 1b's false-positives.

This patch has

-  NS_OVERRIDE void f(const P& p) {}
+  MOZ_OVERRIDE void f(const P& p) {}

which appears to be an incorrect placement?
Attachment #597957 - Flags: review?(benjamin) → review-
Comment on attachment 597958 [details] [diff] [review]
Part 1d, v1: Remove override static analysis tests.

You think these tests aren't valuable because they are testing a compiler behavior that we're not responsible for? It seems that keeping them doesn't hurt anyone.

Updated

7 years ago
Attachment #597959 - Flags: review?(benjamin) → review+

Updated

7 years ago
Attachment #597960 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Comment on attachment 597958 [details] [diff] [review]
> Part 1d, v1: Remove override static analysis tests.
> 
> You think these tests aren't valuable because they are testing a compiler
> behavior that we're not responsible for? It seems that keeping them doesn't
> hurt anyone.

I'm not sure what you mean.

My thinking was, at the point that we no longer use the NS_OVERRIDE static analysis annotation, I didn't think the tests for that particular analysis made much sense.  Keeping the tests means we can't remove the annotation definition; I suppose we could move the definition to a test-only location, but that doesn't seem right.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> Comment on attachment 597957 [details] [diff] [review]
> Part 1c, v1: Replace remaining NS_OVERRIDEs, and fix part 1b's
> false-positives.
> 
> This patch has [...] which appears to be an incorrect placement?

I think I didn't change that one because it's a static-analysis check I intended to delete.
> My thinking was, at the point that we no longer use the NS_OVERRIDE static
> analysis annotation,

Yes, but we're using the MOZ_OVERRIDE notation with the same semantics, and we can just keep the same tests. They will be checked by the compiler hopefully, instead of override.js.
> They will be checked by the compiler hopefully, instead of override.js.

So we want to catch bugs in the compiler?  I guess I didn't think the putative static-analysis tests should be testing that.

In general, if we don't trust the compiler-makers' regression suites, we're in trouble.  And of all the bugs we've seen in compilers, mis-implemented |override| is just about the least of my worries...

Anyway, I can add it back if you want.
Yes, given that we already have the tests and a framework for running them, I'd like to keep them.

Updated

7 years ago
Attachment #597958 - Flags: review?(benjamin) → review-
jcranmer is going to do the NS_OVERRIDE --> MOZ_OVERRIDE switch in bug 702877, and has said he'll do a NS_MUST_OVERRIDE --> MOZ_MUST_BE_OVERRIDDEN switch in bug 767563 (or a dependency).
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 702877
(In reply to Justin Lebar [:jlebar] from comment #9)
> NS_MUST_BE_OVERRIDDEN doesn't make sense on a non-virtual function or on
> functions in a MOZ_FINAL class.

When considering use cases like the SizeOf{Including,Excluding}This functions, it can be specified in a way that does make sense for nonvirtual functions. Final classes of course don't make sense.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Yes, given that we already have the tests and a framework for running them,
> I'd like to keep them.

The test suite that we use the dehydra static checking tests is mildly broken (it checks that an error/warning occurs, and doesn't bother to figure out if it's actually correct); clang has a nice -verify option which they use in their test suites that I've co-opted for further tests, so I've decided just to rip out the entire test suite as exists now.
It's a shame that I let these patches rot waiting for a static analysis fix, then.  :(
You need to log in before you can comment on or make changes to this bug.