551286 - A per function "Final" keyword for C++ code

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Mike Kristoffersen (:MikeK)]

Sometimes we have virtual functions (from interfaces) that have their final implementation in some class, it is currently not possible to prevent these from being overridden by child classes.

It has been proposed that the "virtual" could be removed, that would mean a new version of the NS_IMETHOD prefix, and some people might fail to realize that what they think is the overriding version isn't actually overriding anything...

Comment 1

[Benjamin Smedberg]

While I think this is a good thing, it wouldn't make any sense to remove the `virtual`. Once a method is virtual, it's always virtual.

Comment 2

[Mike Kristoffersen (:MikeK)]

Just another reason to abandon that solution ;)

Comment 3

[Josh Matthews [:jdm]]

Attachment: Analysis pass for overriding functions marked NS_FINAL

First pass at this.  For every function member of a class, all matching ancestor functions are checked for NS_FINAL annotations.

Comment 4

[Benjamin Smedberg]

Needs tests... see xpcom/tests/static-checker for many examples.

Comment 5

[(dormant account)]

Comment on attachment 431604 [details] [diff] [review]
Analysis pass for overriding functions marked NS_FINAL

Josh,
This is pretty good but needs testcases. Your choice of hooking at process_function level rather than process_decl is interesting, it might be more efficient.
Please add the functionality to final.js from bug 425454 instead of adding a new file.


>diff --git a/xpcom/analysis/final-function.js b/xpcom/analysis/final-function.js
>+function ancestorTypes(c)
>+{
>+  ancestors = Array();
>+  for each (let base in c.bases) {
>+      ancestors.concat(ancestorTypes(base.type));
>+      ancestors.push(base.type);
>+  }
>+  return ancestors;
>+}

This is much neater with yield, see http://hg.mozilla.org/mozilla-central/file/c04fe347f85d/xpcom/analysis/override.js

Thanks for the quick patch

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> >+function ancestorTypes(c)
> >+{
> >+  ancestors = Array();

This is cool stuff, great to see this kind of analysis emerge.

Agree on using a generator, but I wanted to leave a tiny drive-by nit about preferring [] to Array() or new Array(). Less spoofable or (to be fair) monkey-patchable, because [] uses the original value of the Array property of the global object, and more optimizable (and optimized in practice).

/be

Comment 7

[Josh Matthews [:jdm]]

Attachment: Analysis pass for overriding functions marked NS_FINAL v2

Now integrated with the existing NS_FINAL pass, and 100% more tests.

Comment 8

[(dormant account)]

Comment on attachment 431826 [details] [diff] [review]
Analysis pass for overriding functions marked NS_FINAL v2

nice!

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f07fe9c3a1a7