Skip to content

Commit

Permalink
Add nullptr checks to shared_ptr conversions (microsoft#17199)
Browse files Browse the repository at this point in the history
We use `if (auto self = weakSelf.get())` in a lot of places.
That assigns the value to `self` and then checks if it's truthy.
Sometimes we need to add a "is (app) closing" check because XAML,
so we wrote something akin to `if (self = ...; !closing)`.

But that's wrong because the correct `if (foo)` is the same as
`if (void; foo)` and not `if (foo; void)` and that meant that
we didn't check for `self`'s truthiness anymore.

This issue became apparent now, because we added a new kind of
delayed callback invocation (which is a lot cheaper).
This made the lack of a `nullptr` check finally obvious.
  • Loading branch information
lhecker authored May 7, 2024
1 parent 9c16c5c commit 6d0342f
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 7 deletions.
4 changes: 2 additions & 2 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
std::chrono::milliseconds{ 100 },
[weakTerminal = std::weak_ptr{ _terminal }, weakThis = get_weak(), dispatcher = _dispatcher]() {
dispatcher.TryEnqueue(DispatcherQueuePriority::Normal, [weakThis]() {
if (const auto self = weakThis.get(); !self->_IsClosing())
if (const auto self = weakThis.get(); self && !self->_IsClosing())
{
self->OutputIdle.raise(*self, nullptr);
}
Expand All @@ -179,7 +179,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_dispatcher,
std::chrono::milliseconds{ 8 },
[weakThis = get_weak()](const auto& update) {
if (auto core{ weakThis.get() }; !core->_IsClosing())
if (auto core{ weakThis.get() }; core && !core->_IsClosing())
{
core->ScrollPositionChanged.raise(*core, update);
}
Expand Down
10 changes: 5 additions & 5 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
dispatcher,
TerminalWarningBellInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() }; !control->_IsClosing())
if (auto control{ weakThis.get() }; control && !control->_IsClosing())
{
control->WarningBell.raise(*control, nullptr);
}
Expand All @@ -258,7 +258,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
dispatcher,
ScrollBarUpdateInterval,
[weakThis = get_weak()](const auto& update) {
if (auto control{ weakThis.get() }; !control->_IsClosing())
if (auto control{ weakThis.get() }; control && !control->_IsClosing())
{
control->_throttledUpdateScrollbar(update);
}
Expand Down Expand Up @@ -301,7 +301,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_originalSelectedSecondaryElements.Append(e);
}
ContextMenu().Closed([weakThis = get_weak()](auto&&, auto&&) {
if (auto control{ weakThis.get() }; !control->_IsClosing())
if (auto control{ weakThis.get() }; control && !control->_IsClosing())
{
const auto& menu{ control->ContextMenu() };
menu.PrimaryCommands().Clear();
Expand All @@ -317,7 +317,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
});
SelectionContextMenu().Closed([weakThis = get_weak()](auto&&, auto&&) {
if (auto control{ weakThis.get() }; !control->_IsClosing())
if (auto control{ weakThis.get() }; control && !control->_IsClosing())
{
const auto& menu{ control->SelectionContextMenu() };
menu.PrimaryCommands().Clear();
Expand Down Expand Up @@ -544,7 +544,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}

_searchBox->Open([weakThis = get_weak()]() {
if (const auto self = weakThis.get(); !self->_IsClosing())
if (const auto self = weakThis.get(); self && !self->_IsClosing())
{
self->_searchBox->SetFocusOnTextbox();
self->_refreshSearch();
Expand Down

0 comments on commit 6d0342f

Please sign in to comment.