Skip to content

Added virtual dtor for dialog #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

judicaelclair
Copy link

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

Fixes bug detected by gcc's address sanitizer.
@samhocevar
Copy link
Owner

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
Copy link
Owner

samhocevar commented Aug 4, 2022

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
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants