[PATCH] D83452: [DWARFYAML] Virtual functions should be overridden in derived class.

[Xing GUO via Phabricator via llvm-commits]

Higuoxing added a comment.

In D83452#2141125 <https://reviews.llvm.org/D83452#2141125>, @grimar wrote:

> Also, I find the description a bit confusing:
>
> > Virtual functions should be overridden in the derived class
>
> Isn't this change (virtual->override) here a no-op to improve the code style?
>  All these funtions are anyways overriden, with the `override` word or without it.


Yes, I know these functions are overridden with or without the `override` key word. Because the `DIEFixupVisitor` class is derived from `DWARFYAML::Vistor`, and those `onXXXX()` functions are declared as virtual functions in `DWARFYAML::Vistor` class. I want to make it stricter since if we use `override` keyword to override an non-existing virtual function, compiler will complain about it. For example, if we change the signature of functions in derived class by mistake, the compiler will spot it for us.

As for the change from `private:` to `protected:`, that's because these functions in `DWARFYAML::Vistor` and `DWARFYAML::DumpVistor` are declared in `protected` keyword, I want to make them consistent. Sorry for my poor expressions :( Does it make sense now?

In D83452#2141126 <https://reviews.llvm.org/D83452#2141126>, @jhenderson wrote:

> Some points:
>
> 1. The `virtual` on the old version didn't declare a new virtual class. In face, it was actually superfluous from a strict point of view. `virtual` is propagated to all sub-class versions of functions with the same signature, regardless of accessor type. Prior to C++11, it was traditionally used to show that a sub-class function was an implementation of a virtual function in the point.
> 2. C++11 introduced the `override` specifier. It doesn't make a function any more or less virtual than it would have been before. All it does is require that the function overrides a virtual function in the parent class.


Yes! That's exactly what I want to do! See my comments for @grimar.

> 3. There's no need to change from `private` to `protected`. This just affects how functions can be called from the outside world - it doesn't affect the `virtual` nature of functions.

I want to make the coding style consistent since these functions in `DWARFYAML::DIEFixupVisitor` and `DWARFYAML::Visitor` are declared in `protected` keyword.

> That all being said, there's nothing wrong with this change, since you're working in the area, in my opinion. Using `override` is always a good idea since it provides safety guarantees (rather than potentially accidentally creating a new virtual function). You just need to update your description and summary accordingly (something like "use override instead of virtual for better safety").




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83452/new/

https://reviews.llvm.org/D83452