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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.
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.

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