[PATCH] D121993: Mark derived destructors as `override`

[Antonio Frighetto via Phabricator via llvm-commits]

antoniofrighetto added a comment.

@dblaikie: That's true, admittedly, had not noticed the protected dtor (which works as well). Fixed it up, thanks for contextualizing.



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:227
   SCEVPredicateKind Kind;
-  ~SCEVPredicate() = default;
+  virtual ~SCEVPredicate() = default;
   SCEVPredicate(const SCEVPredicate &) = default;
----------------
dblaikie wrote:
> antoniofrighetto wrote:
> > dblaikie wrote:
> > > Why is this being made virtual?
> > Might be wrong, but, considering that `SCEVPredicate` is being inherited, the order of destruction object should be carried out correctly, when a derived object goes out of scope (even if no instance of the derived object is reachable through a pointer to the base class) . That actually should be added at every level of the hirearchy.
> If the type isn't owned/destroyed dynamically, then it isn't necessary to make the dtor virtual - that's why the dtor's protected, so it can't be destroyed dynamically.
> 
> eg, this code is OK:
> ```
> struct base {
>   virtual void f();
> protected:
>   ~base();
> };
> struct derived final : base {
>   void f();
> };
> void f(base *b) {
>   b->f();
> }
> ...
> derived d;
> f(&d);
> ```
> The destruction is non-polymorphic/non-dynamic even though the usage may be dynamic.
> 
> This class is designed to work in this way & Clang's warnings have been implemented to allow this usage & avoiding dynamic dispatch overhead where it isn't needed.
That's true, admittedly, I didn't notice the protected dtor.


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

https://reviews.llvm.org/D121993