pbrt::syntactic::Object and others does not have virtual destructor

[syoyo]

For example, Object has virtual methods but no virtual destructor.

https://github.com/ingowald/pbrt-parser/blob/master/pbrtParser/include/pbrtParser/Scene.h#L1003

Clang(9.0) with increased warnings(e.g. -Wall) reports an warning about it:

Here is minimal reproducible code based on pbrtParser:

// clang++ -Wall -std=c++11 
#include <string>
#include <memory>
#include <cstdio>
#include <cstdlib>
#include <iostream>
#include <map>

struct Object
{
  Object(const std::string &name) : name_(name) {}

  // virtual ~Object() { std::cout << "dtor" << "\n"; }

  std::string name_;

  virtual std::string toString(const int depth = 0) const;

};

struct Scene
{
  Scene() : o(std::make_shared<Object>("<root>")) {
  }

  virtual ~Scene();

  std::shared_ptr<Object> o;
};

Scene::~Scene() {
  o = nullptr;
}

int main(int argc, char **argv)
{
  (void)argc;
  (void)argv;

  std::map<std::string, std::shared_ptr<Scene>> pmap;

  pmap["root"] = std::make_shared<Scene>();

  std::cout << "name = " << pmap["root"]->o->name_ << "\n";

  return 0;
}

In file included from main.cc:1:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/string:41:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/allocator.h:46:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/x86_64-linux-gnu/c++/9/bits/c++allocator.h:33:
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:153:4: warning: destructor called on non-final 'Object' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
        { __p->~_Up(); }
          ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/alloc_traits.h:497:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<Object>::destroy<Object>' requested here
        { __a.destroy(__p); }
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr_base.h:557:28: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<Object> >::destroy<Object>' requested here
        allocator_traits<_Alloc>::destroy(_M_impl._M_alloc(), _M_ptr());
                                  ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr_base.h:543:2: note: in instantiation of member function 'std::_Sp_counted_ptr_inplace<Object, std::allocator<Object>, __gnu_cxx::_S_atomic>::_M_dispose' requested here
        _Sp_counted_ptr_inplace(_Alloc __a, _Args&&... __args)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr_base.h:680:6: note: in instantiation of function template specialization 'std::_Sp_counted_ptr_inplace<Object, std::allocator<Object>, __gnu_cxx::_S_atomic>::_Sp_counted_ptr_inplace<char const (&)[7]>' requested here
            _Sp_cp_type(__a._M_a, std::forward<_Args>(__args)...);
            ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr_base.h:1344:14: note: in instantiation of function template specialization 'std::__shared_count<__gnu_cxx::_S_atomic>::__shared_count<Object, std::allocator<Object>, char const (&)[7]>' requested here
        : _M_ptr(), _M_refcount(_M_ptr, __tag, std::forward<_Args>(__args)...)
                    ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr.h:359:4: note: in instantiation of function template specialization 'std::__shared_ptr<Object, __gnu_cxx::_S_atomic>::__shared_ptr<std::allocator<Object>, char const (&)[7]>' requested here
        : __shared_ptr<_Tp>(__tag, std::forward<_Args>(__args)...)
          ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr.h:701:14: note: in instantiation of function template specialization 'std::shared_ptr<Object>::shared_ptr<std::allocator<Object>, char const (&)[7]>' requested here
      return shared_ptr<_Tp>(_Sp_alloc_shared_tag<_Alloc>{__a},
             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/shared_ptr.h:717:19: note: in instantiation of function template specialization 'std::allocate_shared<Object, std::allocator<Object>, char const (&)[7]>' requested here
      return std::allocate_shared<_Tp>(std::allocator<_Tp_nc>(),
                  ^
main.cc:22:20: note: in instantiation of function template specialization 'std::make_shared<Object, char const (&)[7]>' requested here
  Scene() : o(std::make_shared<Object>("<root>")) {
                   ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:153:10: note: qualify call to silence this warning
        { __p->~_Up(); }

https://stackoverflow.com/questions/10024796/c-virtual-functions-but-no-virtual-destructors

No explicit virtual destructor may give unexpected behavior, so it would be better to define virtual destructor explicitly as done in Shape:

pbrt-parser/pbrtParser/include/pbrtParser/Scene.h

Line 741 in 75c7b21

virtual ~Shape() {}

( Same modification may be required to other classes(e.g. Camera)) )

[szellmann]

👍 that would be nice to do a pass for such issues; also rule-of-three/rule-of-five consistency I saw that this is also not consistent everywhere. Those can often lead to subtle problems.

[syoyo]

If there is no strong reason not to define virtual destructors and having default virtual destructor is enough, I can send a PR of adding

virtual ~Object() = default
(define default virtual destructor)

to each structs.

https://en.cppreference.com/w/cpp/language/destructor