[webkit-dev] virtual destructor annotations in subclasses

[Yury Semikhatsky]

Hi all,

This question came up in a code review where I annotated subclass's
destructor with 'override':

class SuperClass {
public:
  virtual ~SuperClass() {}
};

class Subclass : public SuperClass {
public:
  ~Subclass() *override*;
};

The style guide recommends

annotating overriden methods with either 'override' or 'final'. At the same
time with a very few exceptions, the vast majority of the subclasses in
WebKit annotate their destructors with 'virtual' keyword. It might be
because some of the code predates c++11 and virtual was the default
choice back then. In any case it prevents the compiler from generating
errors when super destructor is accidentally not made virtual.

So my question is if there is a reason to exempt destructors from the above
rule and mark them virtual in subclasses or they should be annotated with
override/final similar to other virtual methods?

In the latter case we could update existing code once  by automatically
generating fix with clang-tidy.

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

Re: [webkit-dev] virtual destructor annotations in subclasses

[Yusuke Suzuki]

While I don’t have any opinion about adding override on methods, I would like 
to see more `final` annotation in derived classes’ header when the classes are 
final and inherits something.
In JSC, we leverage this `final` information in `jsCast<>` and `jsDynamicCast` 
to optimize the generated code[1]. So, annotating `final` on JS classes 
actually changes the generated code and makes it optimized.

[1]: If the given JS class is annotated as `final`, we can skip traversing 
subclasses when doing `jsDynamicCast`. We can use this 
information by `std::is_final`.

-Yusuke

> On Oct 8, 2019, at 19:15, Ryosuke Niwa  wrote:
> 
> On Tue, Oct 8, 2019 at 4:24 PM Yury Semikhatsky  > wrote:
> 
> This question came up in a code review where I annotated subclass's 
> destructor with 'override':
> 
> class SuperClass {
> public:
>   virtual ~SuperClass() {}
> };
> 
> class Subclass : public SuperClass {
> public:
>   ~Subclass() override;
> };
> 
> The style guide recommends 
>  
> annotating overriden methods with either 'override' or 'final'. At the same 
> time with a very few exceptions, the vast majority of the subclasses in 
> WebKit annotate their destructors with 'virtual' keyword. It might be because 
> some of the code predates c++11 and virtual was the default choice back then. 
> In any case it prevents the compiler from generating errors when super 
> destructor is accidentally not made virtual.
> 
> So my question is if there is a reason to exempt destructors from the above 
> rule and mark them virtual in subclasses or they should be annotated with 
> override/final similar to other virtual methods?
> 
> In the latter case we could update existing code once  by automatically 
> generating fix with clang-tidy.
> 
> I don't have an opinion either way about the actual style but I'm against 
> adding override everywhere en masse the same way we don't land a bunch of 
> whitespace or other style tidy ups. If we were to decide that we want to add 
> override, that should happen over time as people touch different classes and 
> files.
> 
> - R. Niwa
> 
> ___
> 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] virtual destructor annotations in subclasses

[Ryosuke Niwa]

On Tue, Oct 8, 2019 at 4:24 PM Yury Semikhatsky  wrote:

>
> This question came up in a code review where I annotated subclass's
> destructor with 'override':
>
> class SuperClass {
> public:
>   virtual ~SuperClass() {}
> };
>
> class Subclass : public SuperClass {
> public:
>   ~Subclass() *override*;
> };
>
> The style guide recommends
> 
> annotating overriden methods with either 'override' or 'final'. At the same
> time with a very few exceptions, the vast majority of the subclasses in
> WebKit annotate their destructors with 'virtual' keyword. It might be
> because some of the code predates c++11 and virtual was the default
> choice back then. In any case it prevents the compiler from generating
> errors when super destructor is accidentally not made virtual.
>
> So my question is if there is a reason to exempt destructors from the
> above rule and mark them virtual in subclasses or they should be annotated
> with override/final similar to other virtual methods?
>
> In the latter case we could update existing code once  by automatically
> generating fix with clang-tidy.
>

I don't have an opinion either way about the actual style but I'm against
adding override everywhere en masse the same way we don't land a bunch of
whitespace or other style tidy ups. If we were to decide that we want to
add override, that should happen over time as people touch different
classes and files.

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