Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Michael Catanzaro]

On Fri, 2016-03-04 at 14:04 -0800, Darin Adler wrote:
> I’d like to learn more about this. Are the relevant warnings on for
> all the WebKit project’s compilers, ports, and build systems?

See:

https://bugs.webkit.org/show_bug.cgi?id=153695
https://bugs.webkit.org/show_bug.cgi?id=155048

GCC and Clang both have these warnings (under the same names). I don't
know about MSVCC or XCode.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

> On Mar 4, 2016, at 10:51 AM, Michael Catanzaro  wrote:
> 
> We had a GTK port bug caused by a missing virtual destructor recently.

Oh no!

> GCC and Clang have warnings for this, which I think should be enabled, but 
> apparently were not or somehow didn't work.

I’d like to learn more about this. Are the relevant warnings on for all the 
WebKit project’s compilers, ports, and build systems?

— Darin___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

> On Mar 4, 2016, at 10:51 AM, Michael Catanzaro  wrote:
> 
>> - Style guide should encourage programmers to use final instead of override 
>> whenever possible. I suspect many of the functions that currently are tagged 
>> override should be final instead. Agreed?
> 
> What's your reasoning for this suggestion?


These are internal interfaces within the project. If we later find out we need 
to override in a derived class, at that time we can change final to override.

As long as we don’t need to override in a derived class, having something 
marked final instead of override lets the compiler do more optimization. And it 
tells someone reading the code that there is no further overriding in derived 
classes. Also, the word final is shorter.

It’s similar to why we want people to make things as private as possible, 
preferring private to protected and protected to public. We can always make 
them more private later.

— Darin___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

I think when de-finalizing the class we likely don't want to de-finalize all 
the overrides. Only ones where we need to override.

— Darin

Sent from my iPhone

> On Mar 4, 2016, at 10:39 AM, Konstantin Tokarev  wrote:
> 
> 04.03.2016, 21:32, "Darin Adler" :
>> Here are three other loose ends I am thinking about:
>> 
>> - Style guide should say that in a class marked final, virtual functions 
>> should all be marked final, not override and certainly not virtual. Agreed?
> 
> I agree about virtual, but not sure about override. Class is already final so 
> any code trying to override memeber already doesn't compile, OTOH if later we 
> decide to de-finilize class it would require a lot of line changes.
> 
> But I agree that all-final style has certain visual appeal.
> 
>> Can we check that with the script? 
> 
> It's certainly possible.
> 
>> Apply that rule globally like we just did with the other rules with 
>> do-webcore-rename?
>> 
>> - Style guide should encourage programmers to use final instead of override 
>> whenever possible. I suspect many of the functions that currently are tagged 
>> override should be final instead. Agreed?
>> 
>> - Style guide should discourage virtual on destructors where the destructor 
>> is already virtual due to a base class. This is now more consistent with the 
>> use of virtual on other member functions, final would be on the class, 
>> override doesn’t need to be stated. Agreed?
>> 
>> — 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] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Michael Catanzaro]

On Fri, 2016-03-04 at 10:32 -0800, Darin Adler wrote:
> Here are three other loose ends I am thinking about:
> 
> - Style guide should say that in a class marked final, virtual
> functions should all be marked final, not override and certainly not
> virtual. Agreed? Can we check that with the script? Apply that rule
> globally like we just did with the other rules with do-webcore-
> rename?

I kinda prefer the other way around, to use "override" for virtual
functions even in final classes, as it's a bit more readable IMO.
(There's absolutely no difference between override and final in this
case, right?) But I think it doesn't really matter.

> - Style guide should encourage programmers to use final instead of
> override whenever possible. I suspect many of the functions that
> currently are tagged override should be final instead. Agreed?

What's your reasoning for this suggestion?

> - Style guide should discourage virtual on destructors where the
> destructor is already virtual due to a base class. This is now more
> consistent with the use of virtual on other member functions, final
> would be on the class, override doesn’t need to be stated. Agreed?

We just need to make the style checker's warning clear to ensure
programmers don't improperly remove the destructor declaration in
response to the style checker's warning. We had a GTK port bug caused
by a missing virtual destructor recently... GCC and Clang have warnings
for this, which I think should be enabled, but apparently were not or
somehow didn't work.

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Konstantin Tokarev]

04.03.2016, 21:32, "Darin Adler" :
> Here are three other loose ends I am thinking about:
>
> - Style guide should say that in a class marked final, virtual functions 
> should all be marked final, not override and certainly not virtual. Agreed?

I agree about virtual, but not sure about override. Class is already final so 
any code trying to override memeber already doesn't compile, OTOH if later we 
decide to de-finilize class it would require a lot of line changes.

But I agree that all-final style has certain visual appeal.

> Can we check that with the script? 

It's certainly possible.

>Apply that rule globally like we just did with the other rules with 
>do-webcore-rename?
>
> - Style guide should encourage programmers to use final instead of override 
> whenever possible. I suspect many of the functions that currently are tagged 
> override should be final instead. Agreed?
>
> - Style guide should discourage virtual on destructors where the destructor 
> is already virtual due to a base class. This is now more consistent with the 
> use of virtual on other member functions, final would be on the class, 
> override doesn’t need to be stated. Agreed?
>
> — 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] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

Here are three other loose ends I am thinking about:

- Style guide should say that in a class marked final, virtual functions should 
all be marked final, not override and certainly not virtual. Agreed? Can we 
check that with the script? Apply that rule globally like we just did with the 
other rules with do-webcore-rename?

- Style guide should encourage programmers to use final instead of override 
whenever possible. I suspect many of the functions that currently are tagged 
override should be final instead. Agreed?

- Style guide should discourage virtual on destructors where the destructor is 
already virtual due to a base class. This is now more consistent with the use 
of virtual on other member functions, final would be on the class, override 
doesn’t need to be stated. Agreed?

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Konstantin Tokarev]

04.03.2016, 20:06, "Konstantin Tokarev" :
> 04.03.2016, 16:21, "Konstantin Tokarev" :
>>  03.03.2016, 22:35, "Darin Adler" :
>>>   OK!
>>>
>>>   Do we have volunteers to:
>>>
>>>   1) update the style guide webpage
>>>   2) update check-webkit-style
>>>   3) physically restrain me from turning do-webcore-rename into a perl 
>>> script that does this all the code in the entire source tree all at one go, 
>>> since that would be a bad idea, right?
>>
>>  I volunteer for (2).
>
> Here is my attempt: https://bugs.webkit.org/show_bug.cgi?id=155017
>
> I've borrowed 'readability/inheritance' name from cpplint.py [1], from which 
> cpp.py originates. I've also added two more checks, implemented by 
> cpplint.py: avoid "virtual" when there is "final", and avoid "override" when 
> there is "final".
>
> [1] https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py

However, this check still has a minor problem: if function declaration is 
definition, like here:

virtual f()
{
}

and patch modifies only first line

-virtual f()
+virtual f() override
{
}

error is not detected, because checker treats f as unfinished (inside patch 
borders). However, it should affect other checks as well.

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

> On Mar 4, 2016, at 9:11 AM, Antonio Gomes  wrote:
> 
> It is a good follow up

I agree.

> once the first patch bakes for a while

I don’t think we need “bake time” for this. It’s super-straightforward. I’m 
going to do it right now.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Antonio Gomes]

On a side node, there are lots of lines like

virtual void foo(..) override final;

.. ending up like:

void foo(..) override final;

Ideally though, "override" could also get removed and it would read as

void foo(..) final;

It is a good follow up once the first patch bakes for a while in ToT
(without regression).

On Fri, Mar 4, 2016 at 12:47 PM, Darin Adler  wrote:
>> On Mar 4, 2016, at 6:54 AM, Konstantin Tokarev  wrote:
>>
>> I have WebCore patch ready for upload.
>
> Yes, I had already done this last night 
> . Just haven’t landed it yet 
> because tiled-drawing tests were failing. Fixing that now.
>
> — Darin
> ___
> 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] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

> On Mar 4, 2016, at 9:03 AM, Konstantin Tokarev  wrote:
> 
> I've done different thing - added override specifiers wherever clang deduces 
> they are needed.

Sounds good.

> Here is my patch: https://bugs.webkit.org/show_bug.cgi?id=155021

I’m looking now.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Konstantin Tokarev]

04.03.2016, 19:47, "Darin Adler" :
>>  On Mar 4, 2016, at 6:54 AM, Konstantin Tokarev  wrote:
>>
>>  I have WebCore patch ready for upload.
>
> Yes, I had already done this last night 
> . Just haven’t landed it yet 
> because tiled-drawing tests were failing. Fixing that now.

I've done different thing - added override specifiers wherever clang deduces 
they are needed.

Here is my patch: https://bugs.webkit.org/show_bug.cgi?id=155021

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

> On Mar 4, 2016, at 6:54 AM, Konstantin Tokarev  wrote:
> 
> I have WebCore patch ready for upload.

Yes, I had already done this last night 
. Just haven’t landed it yet 
because tiled-drawing tests were failing. Fixing that now.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Michael Catanzaro]

On Fri, 2016-03-04 at 17:54 +0300, Konstantin Tokarev wrote:
> clang-modernize can do that automatically, I have WebCore patch ready
> for upload.

clang. :)

Let's do this?
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Konstantin Tokarev]

04.03.2016, 17:53, "Michael Catanzaro" :
> On Thu, 2016-03-03 at 11:38 -0800, Geoffrey Garen wrote:
>>  I volunteer for any future needs in the physical restraint department
>>  -- but in this case, I think (3) sounds like a good idea.
>
> Allowing Darin to perform (3) sounds good to me, what could possibly go
> wrong? Some folks will have to rebase patches, but that should not be
> difficult
>
> We could also really go the full mile and start using override wherever
> possible. I guess a script to make that change could be written by
> turning on the GCC -Wsuggest-override warning, and parsing the compiler
> output to get line numbers that need fixed

clang-modernize can do that automatically, I have WebCore patch ready for 
upload.

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Michael Catanzaro]

On Thu, 2016-03-03 at 11:38 -0800, Geoffrey Garen wrote:
> I volunteer for any future needs in the physical restraint department
> -- but in this case, I think (3) sounds like a good idea.

Allowing Darin to perform (3) sounds good to me, what could possibly go
wrong? Some folks will have to rebase patches, but that should not be
difficult

We could also really go the full mile and start using override wherever
possible. I guess a script to make that change could be written by
turning on the GCC -Wsuggest-override warning, and parsing the compiler
output to get line numbers that need fixed

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Konstantin Tokarev]

03.03.2016, 22:35, "Darin Adler" :
> OK!
>
> Do we have volunteers to:
>
> 1) update the style guide webpage
> 2) update check-webkit-style
> 3) physically restrain me from turning do-webcore-rename into a perl script 
> that does this all the code in the entire source tree all at one go, since 
> that would be a bad idea, right?

I volunteer for (2).

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Saam barati]

(3) seems like a good idea to me!
I’ll volunteer for (1)

Saam
> On Mar 3, 2016, at 11:35 AM, Darin Adler  wrote:
> 
> 3) physically restrain me from turning do-webcore-rename into a perl script 
> that does this all the code in the entire source tree all at one go, since 
> that would be a bad idea, right?

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Geoffrey Garen]

I volunteer for any future needs in the physical restraint department -- but in 
this case, I think (3) sounds like a good idea.

Geoff

> On Mar 3, 2016, at 11:35 AM, Darin Adler  wrote:
> 
> OK!
> 
> Do we have volunteers to:
> 
> 1) update the style guide webpage
> 2) update check-webkit-style
> 3) physically restrain me from turning do-webcore-rename into a perl script 
> that does this all the code in the entire source tree all at one go, since 
> that would be a bad idea, right?
> 
> — Darin
> ___
> 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] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

OK!

Do we have volunteers to:

1) update the style guide webpage
2) update check-webkit-style
3) physically restrain me from turning do-webcore-rename into a perl script 
that does this all the code in the entire source tree all at one go, since that 
would be a bad idea, right?

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Andreas Kling]

Huh, I thought we had already decided on this and have been writing new code in 
this style :)

Andreas

> On Mar 3, 2016, at 8:24 PM, Brent Fulgham  wrote:
> 
> +1. I am in favor of this as well!
> 
> -Brent
> 
>> On Mar 3, 2016, at 11:23 AM, Saam barati  wrote:
>> 
>> +1.
>> I like how “override” only reads.
>> 
>> Saam
>> 
>>> On Mar 3, 2016, at 9:54 AM, Ryosuke Niwa  wrote:
>>> 
>>> I think "virtual" + "override" is more of a historical artifact than
>>> the preferred style because we used to have OVERRIDE macro before all
>>> compilers supported C++11.  I think we should just use only "override"
>>> going forward.
>>> - R. Niwa
>>> 
>>> 
>>> On Thu, Mar 3, 2016 at 9:38 AM, Darin Adler  wrote:
 Antti proposed using only “override” a while back since it’s less verbose 
 and still unambiguous. I don’t think we reached consensus on which style 
 to prefer for the project, though.
 
 — Darin
 ___
 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
>> 
>> ___
>> 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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Brent Fulgham]

+1. I am in favor of this as well!

-Brent

> On Mar 3, 2016, at 11:23 AM, Saam barati  wrote:
> 
> +1.
> I like how “override” only reads.
> 
> Saam
> 
>> On Mar 3, 2016, at 9:54 AM, Ryosuke Niwa  wrote:
>> 
>> I think "virtual" + "override" is more of a historical artifact than
>> the preferred style because we used to have OVERRIDE macro before all
>> compilers supported C++11.  I think we should just use only "override"
>> going forward.
>> - R. Niwa
>> 
>> 
>> On Thu, Mar 3, 2016 at 9:38 AM, Darin Adler  wrote:
>>> Antti proposed using only “override” a while back since it’s less verbose 
>>> and still unambiguous. I don’t think we reached consensus on which style to 
>>> prefer for the project, though.
>>> 
>>> — Darin
>>> ___
>>> 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
> 
> ___
> 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] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Saam barati]

+1.
I like how “override” only reads.

Saam

> On Mar 3, 2016, at 9:54 AM, Ryosuke Niwa  wrote:
> 
> I think "virtual" + "override" is more of a historical artifact than
> the preferred style because we used to have OVERRIDE macro before all
> compilers supported C++11.  I think we should just use only "override"
> going forward.
> - R. Niwa
> 
> 
> On Thu, Mar 3, 2016 at 9:38 AM, Darin Adler  wrote:
>> Antti proposed using only “override” a while back since it’s less verbose 
>> and still unambiguous. I don’t think we reached consensus on which style to 
>> prefer for the project, though.
>> 
>> — Darin
>> ___
>> 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

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Michael Catanzaro]

On Thu, 2016-03-03 at 09:54 -0800, Ryosuke Niwa wrote:
> I think "virtual" + "override" is more of a historical artifact than
> the preferred style because we used to have OVERRIDE macro before all
> compilers supported C++11.  I think we should just use only
> "override"
> going forward.
> - R. Niwa

I agree, "virtual" in subclasses adds no value, it is simply redundant
with "override"... we should omit it in new files.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Ryosuke Niwa]

I think "virtual" + "override" is more of a historical artifact than
the preferred style because we used to have OVERRIDE macro before all
compilers supported C++11.  I think we should just use only "override"
going forward.
- R. Niwa


On Thu, Mar 3, 2016 at 9:38 AM, Darin Adler  wrote:
> Antti proposed using only “override” a while back since it’s less verbose and 
> still unambiguous. I don’t think we reached consensus on which style to 
> prefer for the project, though.
>
> — Darin
> ___
> 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] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Darin Adler]

Antti proposed using only “override” a while back since it’s less verbose and 
still unambiguous. I don’t think we reached consensus on which style to prefer 
for the project, though.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'?

[Konstantin Tokarev]

Hello,

Right now there is a lot of code which uses both 'virtual' and 'override' on 
overridden methods, but there is also code (e.g., in B3) which uses only 
'override'. check-webkit-style accepts both styles.

Which style is preferred in new code?

-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev