'final' class specifier and 'final' method specifier
Hi WebKit devs, 'final' keyword can be applied to both a class and a method (non-static member function). I don't know correct terminology for them. I call them 'final' class specifier and 'final' method specifier. I'm now going to update WebKit Code Style Guidelines for using 'final' class specifier for all classes which has no derived classes in Bug 192844. https://bugs.webkit.org/show_bug.cgi?id=192844 I believe this is not controversial. This is not a topic I'd like to discuss today. I'd like to discuss which 'override' or 'final' should be used for methods. Currently, WebKit is using both inconsistently. For example, CSSFontFaceSet class is marked 'final' and its methods are marked 'final', while CSSFontFaceSource class is marked 'final' and its methods are marked 'override'. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/CSSFontFaceS... https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/CSSFontFaceS... According to WebKit Code Style Guidelines, 'final' method specifier is preferable to 'override' if possible. There is a previous discussion thread. https://webkit.org/code-style-guidelines/#override-methods https://lists.webkit.org/pipermail/webkit-dev/2016-March/028022.html I'd like to change this because 'final' doesn't necessarily imply 'override'. See the following stackoverflow: https://stackoverflow.com/questions/29412412/does-final-imply-override IIUC, 'final' method specifier can be useful for such like the following case: class B { public: virtual foo(); virtual bar(); }; class B1 : public B { foo() final; }; class D final : B1 { bar() override; }; The 'final' method specifier indicates D can't override the method foo. Thus, using 'final' method specifier implies other virtual functions can be overridden. I'm not sure my understanding right. Please correct me.
On Dec 19, 2018, at 1:52 AM, Fujii Hironori <fujii.hironori@gmail.com> wrote:
I'd like to change this because 'final' doesn't necessarily imply 'override'. See the following stackoverflow: https://stackoverflow.com/questions/29412412/does-final-imply-override
I’d be happy to require both final and override if there was any benefit to doing so. I read it fairly carefully, and I don’t think it says anything meaningful for us. Maybe we can find a real example in WebKit code where there’s a “final” and if we added “override”, it would have caught a mistake that “final” won’t catch? — Darin
19.12.2018, 21:02, "Darin Adler" <darin@apple.com>:
On Dec 19, 2018, at 1:52 AM, Fujii Hironori <fujii.hironori@gmail.com> wrote:
I'd like to change this because 'final' doesn't necessarily imply 'override'. See the following stackoverflow: https://stackoverflow.com/questions/29412412/does-final-imply-override
I’d be happy to require both final and override if there was any benefit to doing so.
I read it fairly carefully, and I don’t think it says anything meaningful for us. Maybe we can find a real example in WebKit code where there’s a “final” and if we added “override”, it would have caught a mistake that “final” won’t catch?
Adding override to method which already has final specifier doesn't affect anything, because both final and override may ony be used on virtual methods
— Darin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
-- Regards, Konstantin
On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev <annulen@yandex.ru> wrote:
Adding override to method which already has final specifier doesn't affect anything, because both final and override may ony be used on virtual methods
FWIW I prefer override because it's much more clear what that keyword is used for. Michael
19.12.2018, 23:27, "Michael Catanzaro" <mcatanzaro@igalia.com>:
On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev <annulen@yandex.ru> wrote:
Adding override to method which already has final specifier doesn't affect anything, because both final and override may ony be used on virtual methods
FWIW I prefer override because it's much more clear what that keyword is used for.
If class itself has "final" specifier, "override" on methods works in the same way as "final", and I agree that it conveys intention more clear. However, Darin in [1] suggested that we use "final" aggressively to avoid accidentally losing compiler optimization (i.e. devirtualization of call) [1] https://lists.webkit.org/pipermail/webkit-dev/2016-March/028022.html
Michael
-- Regards, Konstantin
Let’s be clear about what we are discussing. The choice is not be between “final” and “override”. The choice is between “final override”, “override final”, and “final” for functions which are both overrides and final. — Darin Sent from my iPhone
On Dec 19, 2018, at 12:27 PM, Michael Catanzaro <mcatanzaro@igalia.com> wrote:
On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev <annulen@yandex.ru> wrote: Adding override to method which already has final specifier doesn't affect anything, because both final and override may ony be used on virtual methods
FWIW I prefer override because it's much more clear what that keyword is used for.
Michael
In that case, I'll point out that C++ Core Guidelines has a rule "Virtual functions should specify exactly one of virtual, override, or final". (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override) Their tl;dr: " • virtual means exactly and only “this is a new virtual function.” • override means exactly and only “this is a non-final overrider.” • final means exactly and only “this is a final overrider.” " FWIW, they also have a rule "Use final sparingly" with the note that "Claims of performance improvements from final should be substantiated." (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-final) Ross On 12/19/18, 1:54 PM, "webkit-dev on behalf of Darin Adler" <webkit-dev-bounces@lists.webkit.org on behalf of darin@apple.com> wrote: Let’s be clear about what we are discussing. The choice is not be between “final” and “override”. The choice is between “final override”, “override final”, and “final” for functions which are both overrides and final. — Darin Sent from my iPhone > On Dec 19, 2018, at 12:27 PM, Michael Catanzaro <mcatanzaro@igalia.com> wrote: > >> On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev <annulen@yandex.ru> wrote: >> Adding override to method which already has final specifier doesn't affect anything, >> because both final and override may ony be used on virtual methods > > FWIW I prefer override because it's much more clear what that keyword is used for. > > Michael _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Thank you very much for the feedbacks. On Thu, Dec 20, 2018 at 4:52 AM Konstantin Tokarev <annulen@yandex.ru> wrote:
19.12.2018, 12:53, "Fujii Hironori" <fujii.hironori@gmail.com>:
I'd like to change this because 'final' doesn't necessarily imply 'override'. See the following stackoverflow: https://stackoverflow.com/questions/29412412/does-final-imply-override
It does imply override, unless it is used in a declaration of new virtual method (which has no practical meaning fwiw)
You are right. C++ allows using 'final' with 'virtual' without overriding. Even though I don't know practical use-cases for it, C++ allows it because 'final' doesn't mean overriding. And, this is the only reason 'final' doesn't necessarily imply override. This is a kind of chicken egg problem. I don't know which is true: 1. C++ allows it because 'final' doesn't mean overriding. 2. 'final' doesn't necessarily imply override because C++ allows it On Thu, Dec 20, 2018 at 6:28 AM Konstantin Tokarev <annulen@yandex.ru> wrote:
19.12.2018, 23:27, "Michael Catanzaro" <mcatanzaro@igalia.com>:
On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev <annulen@yandex.ru> wrote:
Adding override to method which already has final specifier doesn't affect anything, because both final and override may ony be used on virtual methods
FWIW I prefer override because it's much more clear what that keyword is used for.
If class itself has "final" specifier, "override" on methods works in the same way as "final", and I agree that it conveys intention more clear.
I think so, especially after I will update the code style guidelines in Bug 192844. Bug 192844 – Update code style guidelines for using 'final' specifier for all classes which has no derived classes https://bugs.webkit.org/show_bug.cgi?id=192844
However, Darin in [1] suggested that we use "final" aggressively to avoid accidentally losing compiler optimization (i.e. devirtualization of call)
[1] https://lists.webkit.org/pipermail/webkit-dev/2016-March/028022.html
I think this won't be a problem after all classes which has no derived classes are capped with 'final'. On Thu, Dec 20, 2018 at 7:42 AM <Ross.Kirsling@sony.com> wrote:
In that case, I'll point out that C++ Core Guidelines has a rule "Virtual functions should specify exactly one of virtual, override, or final". (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override)
Their tl;dr: " • virtual means exactly and only “this is a new virtual function.” • override means exactly and only “this is a non-final overrider.” • final means exactly and only “this is a final overrider.” "
FWIW, they also have a rule "Use final sparingly" with the note that "Claims of performance improvements from final should be substantiated." (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-final)
C.128 is a same rule with the current WebKit coding style guidelines. But, I think C.128 makes sense with C.139. C.139 is against to Bug 192844. After Bug 192844 update, we will have a lot of 'final' classes, not sparignly.
20.12.2018, 09:17, "Fujii Hironori" <fujii.hironori@gmail.com>:
Thank you very much for the feedbacks.
On Thu, Dec 20, 2018 at 4:52 AM Konstantin Tokarev <annulen@yandex.ru> wrote:
19.12.2018, 12:53, "Fujii Hironori" <fujii.hironori@gmail.com>:
I'd like to change this because 'final' doesn't necessarily imply 'override'. See the following stackoverflow: https://stackoverflow.com/questions/29412412/does-final-imply-override
It does imply override, unless it is used in a declaration of new virtual method (which has no practical meaning fwiw)
You are right. C++ allows using 'final' with 'virtual' without overriding. Even though I don't know practical use-cases for it, C++ allows it because 'final' doesn't mean overriding. And, this is the only reason 'final' doesn't necessarily imply override. This is a kind of chicken egg problem. I don't know which is true:
1. C++ allows it because 'final' doesn't mean overriding. 2. 'final' doesn't necessarily imply override because C++ allows it
I see no problem here, because our code style checker forbids using 'final' and 'virtual' in the same declaration, so it's only allowed in overriding context.
On Thu, Dec 20, 2018 at 6:28 AM Konstantin Tokarev <annulen@yandex.ru> wrote:
19.12.2018, 23:27, "Michael Catanzaro" <mcatanzaro@igalia.com>:
On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev <annulen@yandex.ru> wrote:
Adding override to method which already has final specifier doesn't affect anything, because both final and override may ony be used on virtual methods
FWIW I prefer override because it's much more clear what that keyword is used for.
If class itself has "final" specifier, "override" on methods works in the same way as "final", and I agree that it conveys intention more clear.
I think so, especially after I will update the code style guidelines in Bug 192844.
Bug 192844 – Update code style guidelines for using 'final' specifier for all classes which has no derived classes https://bugs.webkit.org/show_bug.cgi?id=192844
However, Darin in [1] suggested that we use "final" aggressively to avoid accidentally losing compiler optimization (i.e. devirtualization of call)
[1] https://lists.webkit.org/pipermail/webkit-dev/2016-March/028022.html
I think this won't be a problem after all classes which has no derived classes are capped with 'final'.
However, it might be a problem when leaf class with 'final' becomes non-final.
On Thu, Dec 20, 2018 at 7:42 AM <Ross.Kirsling@sony.com> wrote:
In that case, I'll point out that C++ Core Guidelines has a rule "Virtual functions should specify exactly one of virtual, override, or final". (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override)
Their tl;dr: " • virtual means exactly and only “this is a new virtual function.” • override means exactly and only “this is a non-final overrider.” • final means exactly and only “this is a final overrider.” "
FWIW, they also have a rule "Use final sparingly" with the note that "Claims of performance improvements from final should be substantiated." (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-final)
C.128 is a same rule with the current WebKit coding style guidelines. But, I think C.128 makes sense with C.139. C.139 is against to Bug 192844. After Bug 192844 update, we will have a lot of 'final' classes, not sparignly.
Do you have an idea how to automate this? Otherwise we'll never reach the state where all leaf classes are final, because doing it manually will take lots of time, and I see no way to enforce the rule in new code
,
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
-- Regards, Konstantin
On Fri, Dec 21, 2018 at 5:02 AM Konstantin Tokarev <annulen@yandex.ru> wrote:
On Thu, Dec 20, 2018 at 7:42 AM <Ross.Kirsling@sony.com> wrote:
In that case, I'll point out that C++ Core Guidelines has a rule
"Virtual functions should specify exactly one of virtual, override, or final".
( http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override)
Their tl;dr: " • virtual means exactly and only “this is a new virtual function.” • override means exactly and only “this is a non-final overrider.” • final means exactly and only “this is a final overrider.” "
FWIW, they also have a rule "Use final sparingly" with the note that "Claims of performance improvements from final should be substantiated." (http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-final)
C.128 is a same rule with the current WebKit coding style guidelines. But, I think C.128 makes sense with C.139. C.139 is against to Bug 192844. After Bug 192844 update, we will have a lot of 'final' classes, not sparignly.
Do you have an idea how to automate this? Otherwise we'll never reach the state where all leaf classes are final, because doing it manually will take lots of time, and I see no way to enforce the rule in new code
I don't have such intelligent plan. I don't like the divergence between WebKit Code Style Guidelines and WebKit source code. I've often got review feedbacks that I should modernize the code around my change. I'm going to change them manually, and all patches will be checked by reviewer's eyes. There is a previous such effort. Bug 159802 – Add final keyword to WebCore/svg classes https://bugs.webkit.org/show_bug.cgi?id=159802 It seems that I shouldn't change the current 'final' method specifier rule of WebKit. Then, I think the sentences should be revised such like C.128.
19.12.2018, 12:53, "Fujii Hironori" <fujii.hironori@gmail.com>:
I'd like to change this because 'final' doesn't necessarily imply 'override'. See the following stackoverflow: https://stackoverflow.com/questions/29412412/does-final-imply-override
It does imply override, unless it is used in a declaration of new virtual method (which has no practical meaning fwiw)
IIUC, 'final' method specifier can be useful for such like the following case:
class B { public: virtual foo(); virtual bar(); }; class B1 : public B { foo() final; }; class D final : B1 { bar() override; };
The 'final' method specifier indicates D can't override the method foo. Thus, using 'final' method specifier implies other virtual functions can be overridden. I'm not sure my understanding right. Please correct me.
final method specifier on foo doesn't affect other methods anyhow, it just forbids overriding of foo
,
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
-- Regards, Konstantin
participants (5)
-
Darin Adler
-
Fujii Hironori
-
Konstantin Tokarev
-
Michael Catanzaro
-
Ross.Kirsling＠sony.com