samhocevar / portable-file-dialogs

💬 Portable GUI dialogs library, C++11, single-header
Do What The F*ck You Want To Public License
1.03k stars 99 forks source link

Added virtual dtor for dialog #76

Open judicaelclair opened 2 years ago

judicaelclair commented 2 years ago

Fixes bug detected by gcc's address sanitizer:

==261921==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x606000158960 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   64 bytes;
  size of the deallocated type: 16 bytes.
    #0 0x7ffad17cb22f in operator delete(void*, unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:172
    #1 0x5639317e2ba9 in std::default_delete<pfd::internal::dialog>::operator()(pfd::internal::dialog*) const /usr/include/c++/11/bits/unique_ptr.h:85
    #2 0x5639319fdc64 in std::unique_ptr<pfd::internal::dialog, std::default_delete<pfd::internal::dialog> >::~unique_ptr() /usr/include/c++/11/bits/unique_ptr.h:361
samhocevar commented 2 years ago

Good catch! I did not think people would use pfd::internal::dialog directly. Can you please explain what use you have for a std::unique_ptr<pfd::internal::dialog> instead of a more explicit type?

samhocevar commented 2 years ago

I am asking because I am pondering an API change that switches to factories instead of constructors (see the feature/shared-ptr branch), where e.g. pfd::open_file::create() returns a std::shared_ptr<open_file>. It will break your usage unless I take additional steps to allow conversions.

judicaelclair commented 2 years ago

I have a thin wrapper to your library that manages callbacks and only allows at most a single dialog to be open at any time. This wrapper has a handle to pfd::internal::dialog so that you can kill an existing dialog by calling pfd::internal::dialog->kill().

Since you don't appear to be changing the inheritance hierarchy, I could easily adapt my wrapper by changing two lines:

std::unique_ptr<pfd::internal::dialog> dialog_;
dialog_  = std::make_unique<T>(std::forward<Args>(args)...);

to

std::shared_ptr<pfd::internal::dialog> dialog_;
dialog_  = T::create(std::forward<Args>(args)...);

However, I wonder why you want to use factories seeing as you're merely returning the shared pointer and not using it internally?