Closed
Bug 551286
Opened 15 years ago
Closed 15 years ago
A per function "Final" keyword for C++ code
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: MikeK, Assigned: jdm)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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.
Reporter | ||
Comment 2•15 years ago
|
||
Just another reason to abandon that solution ;)
Updated•15 years ago
|
Blocks: static_analyses
Assignee | ||
Comment 3•15 years ago
|
||
First pass at this. For every function member of a class, all matching ancestor functions are checked for NS_FINAL annotations.
Assignee: nobody → josh
Attachment #431604 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #431604 -
Flags: review? → review?(tglek)
Comment 4•15 years ago
|
||
Needs tests... see xpcom/tests/static-checker for many examples.
Flags: in-testsuite?
Comment 5•15 years ago
|
||
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
Attachment #431604 -
Flags: review?(tglek) → review-
Comment 6•15 years ago
|
||
(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
Assignee | ||
Comment 7•15 years ago
|
||
Now integrated with the existing NS_FINAL pass, and 100% more tests.
Attachment #431604 -
Attachment is obsolete: true
Attachment #431826 -
Flags: review?(tglek)
Comment 8•15 years ago
|
||
Comment on attachment 431826 [details] [diff] [review]
Analysis pass for overriding functions marked NS_FINAL v2
nice!
Attachment #431826 -
Flags: review?(tglek) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•