fix wxTipWindow construction sequencing (PR #26071)
Bill Su
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26071
Commit Summary
- 2e51fe3 wxTipWindow: avoid race condition by using two-phase construction
- f005074 wxTipWindow: safe one-phase construction
File Changes
(6 files)
- M include/wx/tipwin.h (43)
- M interface/wx/tipwin.h (69)
- M samples/dialogs/dialogs.cpp (11)
- M samples/dialogs/dialogs.h (2)
- M src/common/cshelp.cpp (6)
- M src/generic/tipwin.cpp (36)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26071.patch
- https://github.com/wxWidgets/wxWidgets/pull/26071.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- dd743a0 wxTipWindow: safe one-phase construction
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- 94a4164 wxTipWindow: safe one-phase construction
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this looks nice, but I think that adding IsOk() or operator!=(nullptr_t) is needed to make using this even more natural.
Also, if the use of unique_ptr for Ref::p is really required, please add a comment explaining why as I just don't see it (and please rename p to something with m_ prefix in any case).
TIA!
> @@ -25,7 +25,77 @@ class wxTipWindow : public wxWindow
{
public:
/**
- Constructor. The tip is shown immediately after the window is constructed.
+ The (weak) reference of class wxTipWindow
+
+ wxTipWindow may close itself, so provide a smart pointer
+ that acts as a weak reference to wxTipWindow.
+
+ This is a move-only type.
+
+ Note that this is not a wxWeakRef<> because this is set
+ to @NULL when wxTipWindow is closed, which may be
+ "long" before wxTipWindow is destroyed, and wxWeakRef<>
+ is set to @NULL on object destruction.
+ */
Please add
⬇️ Suggested change- */ + + @since 3.3.2 + */
here and for the new functions below.
In samples/dialogs/dialogs.cpp:
> );
}
}
void MyFrame::OnUpdateShowTipUI(wxUpdateUIEvent& event)
{
- event.Check(m_tipWindow != nullptr);
+ event.Check(bool(m_tipWindow));
I think this shows that we need IsOk() member function in Ref.
Or, maybe, implement operator!=() for it?
> @@ -314,11 +314,11 @@ bool wxHtmlHelpController::DisplayTextPopup(const wxString& text, const wxPoint&
s_tipWindow->SetTipWindowPtr(nullptr);
s_tipWindow->Close();
}
- s_tipWindow = nullptr;
+ s_tipWindow.Reset();
In theory we could also keep this working and avoid this change by providing overloaded operator=(std::nullptr_t). Not sure if it's really much better than Reset().
> + // Note that this is not a wxWeakRef<> because this is set
+ // to nullptr when wxTipWindow is closed, which may be
+ // "long" before wxTipWindow is destroyed, bug wxWeakRef<>
+ // is set to nullptr on object destruction
+ class Ref
+ {
+ public:
+ Ref() { *p = nullptr; }
+
+ void Reset() { *p = nullptr; }
+
+ explicit operator bool() const { return *p; }
+ wxTipWindow* operator->() const { wxASSERT(*p); return *p; }
+
+ private:
+ std::unique_ptr<wxTipWindow*> p { new wxTipWindow* };
Sorry, I may be missing something obvious but why do we need to use std::unique_ptr here when it looks like we could just have wxTipWindow* m_ptr; and replace *p above with m_ptr?
> + //
+ // Note that this is a move-only type.
+ //
+ // Note that this is not a wxWeakRef<> because this is set
+ // to nullptr when wxTipWindow is closed, which may be
+ // "long" before wxTipWindow is destroyed, bug wxWeakRef<>
+ // is set to nullptr on object destruction
+ class Ref
+ {
+ public:
+ Ref() { *p = nullptr; }
+
+ void Reset() { *p = nullptr; }
+
+ explicit operator bool() const { return *p; }
+ wxTipWindow* operator->() const { wxASSERT(*p); return *p; }
This wxASSERT() is pretty useless, it's going to crash in any case and it doesn't add anything helpful in debug builds (asserting or crashing at this line is about the same).
> @@ -26,15 +26,56 @@ class WXDLLIMPEXP_FWD_CORE wxTipWindowView;
class WXDLLIMPEXP_CORE wxTipWindow : public wxPopupTransientWindow
{
public:
- // the mandatory ctor parameters are: the parent window and the text to
+ // wxTipWindow may close itself, so provide a smart pointer
+ // that acts as a weak reference to wxTipWindow.
+ //
+ // Note that this is a move-only type.
Can it actually be moved? I think you need to provide move ctor/assignment operator (but they can be = default).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In samples/dialogs/dialogs.cpp:
> @@ -2858,21 +2854,20 @@ void MyFrame::OnShowTip(wxCommandEvent& WXUNUSED(event))
}
else
{
- m_tipWindow = new wxTipWindow
+ m_tipWindow = wxTipWindow::New
This line proves that wxTipWindow::Ref is moveable.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -26,15 +26,56 @@ class WXDLLIMPEXP_FWD_CORE wxTipWindowView;
class WXDLLIMPEXP_CORE wxTipWindow : public wxPopupTransientWindow
{
public:
- // the mandatory ctor parameters are: the parent window and the text to
+ // wxTipWindow may close itself, so provide a smart pointer
+ // that acts as a weak reference to wxTipWindow.
+ //
+ // Note that this is a move-only type.
See https://github.com/wxWidgets/wxWidgets/pull/26071/files#r2663388851. (The std::unique_ptr<> provides move ctor/assignment, and any class ctor or assignment automatically uses its members ctor or assignment unless they are blocked somehow. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-zero).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- fcec3ce wxTipWindow: implement code review requests
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -25,7 +25,77 @@ class wxTipWindow : public wxWindow
{
public:
/**
- Constructor. The tip is shown immediately after the window is constructed.
+ The (weak) reference of class wxTipWindow
+
+ wxTipWindow may close itself, so provide a smart pointer
+ that acts as a weak reference to wxTipWindow.
+
+ This is a move-only type.
+
+ Note that this is not a wxWeakRef<> because this is set
+ to @NULL when wxTipWindow is closed, which may be
+ "long" before wxTipWindow is destroyed, and wxWeakRef<>
+ is set to @NULL on object destruction.
+ */
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
In samples/dialogs/dialogs.cpp:
> );
}
}
void MyFrame::OnUpdateShowTipUI(wxUpdateUIEvent& event)
{
- event.Check(m_tipWindow != nullptr);
+ event.Check(bool(m_tipWindow));
implemented operator!=(nullptr_t)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -314,11 +314,11 @@ bool wxHtmlHelpController::DisplayTextPopup(const wxString& text, const wxPoint&
s_tipWindow->SetTipWindowPtr(nullptr);
s_tipWindow->Close();
}
- s_tipWindow = nullptr;
+ s_tipWindow.Reset();
implemented operator=(nullptr_t)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> + //
+ // Note that this is a move-only type.
+ //
+ // Note that this is not a wxWeakRef<> because this is set
+ // to nullptr when wxTipWindow is closed, which may be
+ // "long" before wxTipWindow is destroyed, bug wxWeakRef<>
+ // is set to nullptr on object destruction
+ class Ref
+ {
+ public:
+ Ref() { *p = nullptr; }
+
+ void Reset() { *p = nullptr; }
+
+ explicit operator bool() const { return *p; }
+ wxTipWindow* operator->() const { wxASSERT(*p); return *p; }
wxASSERT() removed
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> + // Note that this is not a wxWeakRef<> because this is set
+ // to nullptr when wxTipWindow is closed, which may be
+ // "long" before wxTipWindow is destroyed, bug wxWeakRef<>
+ // is set to nullptr on object destruction
+ class Ref
+ {
+ public:
+ Ref() { *p = nullptr; }
+
+ void Reset() { *p = nullptr; }
+
+ explicit operator bool() const { return *p; }
+ wxTipWindow* operator->() const { wxASSERT(*p); return *p; }
+
+ private:
+ std::unique_ptr<wxTipWindow*> p { new wxTipWindow* };
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- aae8aed wxTipWindow: implement code review requests
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -26,15 +26,56 @@ class WXDLLIMPEXP_FWD_CORE wxTipWindowView;
class WXDLLIMPEXP_CORE wxTipWindow : public wxPopupTransientWindow
{
public:
- // the mandatory ctor parameters are: the parent window and the text to
+ // wxTipWindow may close itself, so provide a smart pointer
+ // that acts as a weak reference to wxTipWindow.
+ //
+ // Note that this is a move-only type.
Yes, you're right, sorry for overlooking this.
I know that this class is hardly performance-critical, but it still makes me cringe a little that we use an extra heap allocation just to move this movable. It looks like using a raw pointer and deleting special copy member functions and implementing move ones shouldn't be that difficult and would avoid it...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- b19cd01 wxTipWindow: avoid unnecessary heap use
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26071)Great, thanks! Will merge soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.