Bug 192844 – Update code style guidelines for using 'final' specifier for all classes which has no derived classes

• Component: WebKit Website
• Status: NEW

[Fujii Hironori]

Update code style guidelines for using 'final' specifier for all classes which has no derived classes

It seems a good practice to mark all non-base classes with 'final' specifier.

(In reply to Darin Adler from bug #165903 comment #3)
> Another thing that would be nice to do would be marking more classes final.
> Lots of these classes seem to be final. Any class that has no classes
> deriving from it should probably get marked that way. We can always remove
> it later if we want to derive a class from it.

Bug 136540 – Make hash table classes final

Bug 159802 – Add final keyword to WebCore/svg classes

[Yusuke Suzuki]

I would like to note the one interesting optimization done in JSC to encourage `final` use in WebKit.
If the class is annotated as "final", we can ensure that there are no derived class.
This information can be leveraged by type_traits' reflection mechanism (std::is_final) and it can optimize some code in JSC.

If we have `jsDynamicCast<T>`, and T is final class, the check should not traverse the type hierarchy at runtime. We just check the given JSObject's type is the T's classInfo, and if this check fails, we can say that the given JSObject is not a subclass of T.
So, if you put `final` to JS object classes, it can optimize type checks.

https://trac.webkit.org/changeset/229413/webkit

[Fujii Hironori]

Created attachment 358061 [details]
Patch

[Fujii Hironori]

For ease of review, uploaded the markdown.
https://gist.github.com/fujii/789d8f564508d3a59f3799a0457c4b8c

[Konstantin Tokarev]

I'm not sure if it is a good idea to spread this rule on all classes which are not part of any hierarchy, except when we really mean it. This seems to be especially redundant with inner and local classes.

OTOH, I've found a way how to enforce such rule. My approach is following:
* Doxygen is used to generate XML "documentation" for everything in Source directory (GENERATE_XML = YES). Resulting XML files contain information about base and derived classes for every class (CLASS_GRAPH = YES).
* Script is run over all generated *.xml files, printing all classes which are not final (yet), have no derived classes, and have base class. Script can be implemented in any language, but XQuery implementation shows concisely what is done:

for $c in doc($file)/doxygen/compounddef[@kind = "struct" or @kind = "class"]
let $is_final := string($c/@final)
let $classname := $c/compoundname
let $is_leaf_class := count($c/derivedcompoundref) = 0
let $is_derived_class := count($c/basecompoundref) > 0
where $is_final != "yes" and $is_leaf_class and $is_derived_class
return string($classname)

[Fujii Hironori]

(In reply to Konstantin Tokarev from comment #4)
> I'm not sure if it is a good idea to spread this rule on all classes which
> are not part of any hierarchy, except when we really mean it. This seems to
> be especially redundant with inner and local classes.

Bug 136540 marked HashCountedSet, HashMap and HashSet 'final' even though they are not derived classes.
Marking 'final' makes clear those classes can't be derived.

[Darin Adler]

(In reply to Fujii Hironori from comment #5)
> Bug 136540 marked HashCountedSet, HashMap and HashSet 'final' even though
> they are not derived classes.
> Marking 'final' makes clear those classes can't be derived.

I’m not sure I see any benefit to doing that.