Re: [webkit-dev] 'final' class specifier and 'final' method specifier
On Fri, Dec 21, 2018 at 5:02 AM Konstantin Tokarev wrote: > > > > On Thu, Dec 20, 2018 at 7:42 AM 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. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
20.12.2018, 09:17, "Fujii Hironori" : > Thank you very much for the feedbacks. > > On Thu, Dec 20, 2018 at 4:52 AM Konstantin Tokarev wrote: >> 19.12.2018, 12:53, "Fujii Hironori" : >>> 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 wrote: >> 19.12.2018, 23:27, "Michael Catanzaro" : >>> On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev >>> 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 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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
Thank you very much for the feedbacks. On Thu, Dec 20, 2018 at 4:52 AM Konstantin Tokarev wrote: > > 19.12.2018, 12:53, "Fujii Hironori" : > > 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 wrote: > > > 19.12.2018, 23:27, "Michael Catanzaro" : > > On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev > > 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 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. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
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" 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 wrote: > >> On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev 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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
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 wrote: > >> On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev >> 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
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
19.12.2018, 23:27, "Michael Catanzaro" : > On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev > 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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
On Wed, Dec 19, 2018 at 1:58 PM, Konstantin Tokarev 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
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
19.12.2018, 21:02, "Darin Adler" : >> On Dec 19, 2018, at 1:52 AM, Fujii Hironori >> 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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
19.12.2018, 12:53, "Fujii Hironori" :
> 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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] 'final' class specifier and 'final' method specifier
> On Dec 19, 2018, at 1:52 AM, Fujii Hironori 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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] '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/CSSFontFaceSet.h?rev=239365#L93
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/CSSFontFaceSource.h?rev=239365#L83
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.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
