Skip to content
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

Jump to bottom

Convert CommandHistory to a std::shared_ptr #1161

Merged
merged 5 commits into from Jun 19, 2019
Merged

Convert CommandHistory to a std::shared_ptr #1161

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1452071
Changes for Issue #1058 are done in VS2017. Works fine.
ChristophePichaud Jun 7, 2019
4e4b965
calculatePopupSize use a const CommandHistory& and is called from a s…
ChristophePichaud Jun 7, 2019
3f19be4
Some shared_ptr are converted into refrences.
ChristophePichaud Jun 10, 2019
3cd6f1b
Add missing const stuff.
ChristophePichaud Jun 12, 2019
e8104ed
cookedReadData.History() returns a ref. Callers adapted. OK.
ChristophePichaud Jun 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 25 additions & 25 deletions src/host/CommandListPopup.cpp
View file
Open in desktop
Expand Up @@ -50,10 +50,10 @@ static COORD calculatePopupSize(const CommandHistory& history)
return { gsl::narrow<short>(width), height };
}

CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const CommandHistory& history) :
Popup(screenInfo, calculatePopupSize(history)),
CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const std::shared_ptr<CommandHistory>& history) :
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChristophePichaud I haven't looked at the ownership chain closely but I question why CommandHistory needs be a shared_ptr for this class. (and some of the other const shared_ptr references in this PR)
Take a look at this for guidance for shared_ptr.
I could be wrong because I haven't looked at this close enough. The thing to look for here which classes are sharing ownership? Those are the classes that need to have shared_ptrs. If the class doesn't share ownership then they should be raw pointer or references. Hope this makes sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to encapsulate the raw pointer as a smart pointer. You don't have some many choice, it's unique_ptr, weak_ptr or shared_ptr. Ok you can get back a raw pointer but only in small scenarios. I read the guidance and I make the changes. Thanks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have applied the guidance. It's cool.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChristophePichaud The CommandListPopup ctor in this case is taking the shared pointer by const reference. This indicates that this class is not taking part in the ownership. ie - ref counting is not being incremented. So this code does not need to use a shared_ptr.

This code can be left as it was. (taking a const CommandHistory&) The benefit is you are not tying the pointer type to the class. If in the future this code changes and uses a different kind of smart pointer this code would not have to change.

If CommandListPopup really was needed to be part of the ownership chain then this code wouldn't work anyway because it doesn't increment the ref count.

Does this make sense?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CommandListPopup ctor in this case is taking the shared pointer by const reference.

This should be
The CommandListPopup ctor in this case is taking the shared pointer by const reference and the class is storing the variable in a const & in the class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CommandListPopup class accepts the history argument two times. First, its stores it as a shared_ptr for future use and also as a const ref for a simple calculation for the bqse ctor... So the code is correct.
Check twice.
Tell me back ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @dlong11 on this, for this class it needs to have a CommandHistory and it can't be changed after a CommandHistory class is created. The history also shouldn't be destructed while a popup is around so keeping it a CommandHistory& seems more appropriate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @dlong11 and @adiviness, I have made the various changes in the code.
You are right, sometimes, it's better to handle the stuff with references.
Thanks.

Popup(screenInfo, calculatePopupSize(*history)),
_history{ history },
_currentCommand{ std::min(history.LastDisplayed, static_cast<SHORT>(history.GetNumberOfCommands() - 1)) }
_currentCommand{ std::min(history->LastDisplayed, static_cast<SHORT>(history->GetNumberOfCommands() - 1)) }
{
FAIL_FAST_IF(_currentCommand < 0);
_setBottomIndex();
Expand Down Expand Up @@ -106,11 +106,11 @@ NTSTATUS CommandListPopup::_handlePopupKeys(CookedRead& cookedReadData, const wc
break;
case VK_END:
// Move waaay forward, UpdateCommandListPopup() can handle it.
_update((SHORT)(cookedReadData.History().GetNumberOfCommands()));
_update((SHORT)(cookedReadData.History()->GetNumberOfCommands()));
break;
case VK_HOME:
// Move waaay back, UpdateCommandListPopup() can handle it.
_update(-(SHORT)(cookedReadData.History().GetNumberOfCommands()));
_update(-(SHORT)(cookedReadData.History()->GetNumberOfCommands()));
break;
case VK_PRIOR:
_update(-(SHORT)Height());
Expand All @@ -136,13 +136,13 @@ NTSTATUS CommandListPopup::_handlePopupKeys(CookedRead& cookedReadData, const wc

void CommandListPopup::_setBottomIndex()
{
if (_currentCommand < (SHORT)(_history.GetNumberOfCommands() - Height()))
if (_currentCommand < (SHORT)(_history->GetNumberOfCommands() - Height()))
{
_bottomIndex = std::max(_currentCommand, gsl::narrow<SHORT>(Height() - 1i16));
}
else
{
_bottomIndex = (SHORT)(_history.GetNumberOfCommands() - 1);
_bottomIndex = (SHORT)(_history->GetNumberOfCommands() - 1);
}
}

Expand All @@ -151,18 +151,18 @@ NTSTATUS CommandListPopup::_deleteSelection(CookedRead& cookedReadData) noexcept
{
try
{
auto& history = cookedReadData.History();
history.Remove(static_cast<short>(_currentCommand));
auto history = cookedReadData.History();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because cookedReadData.History() returns a CommandHistory& each of the autos in this file need to be auto& to avoid copying the command history.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes...

history->Remove(static_cast<short>(_currentCommand));
_setBottomIndex();

if (history.GetNumberOfCommands() == 0)
if (history->GetNumberOfCommands() == 0)
{
// close the popup
return CONSOLE_STATUS_READ_COMPLETE;
}
else if (_currentCommand >= static_cast<short>(history.GetNumberOfCommands()))
else if (_currentCommand >= static_cast<short>(history->GetNumberOfCommands()))
{
_currentCommand = static_cast<short>(history.GetNumberOfCommands() - 1);
_currentCommand = static_cast<short>(history->GetNumberOfCommands() - 1);
_bottomIndex = _currentCommand;
}

Expand All @@ -181,13 +181,13 @@ NTSTATUS CommandListPopup::_swapUp(CookedRead& cookedReadData) noexcept
{
try
{
auto& history = cookedReadData.History();
auto history = cookedReadData.History();

if (history.GetNumberOfCommands() <= 1 || _currentCommand == 0)
if (history->GetNumberOfCommands() <= 1 || _currentCommand == 0)
{
return STATUS_SUCCESS;
}
history.Swap(_currentCommand, _currentCommand - 1);
history->Swap(_currentCommand, _currentCommand - 1);
_update(-1);
_drawList();
}
Expand All @@ -204,13 +204,13 @@ NTSTATUS CommandListPopup::_swapDown(CookedRead& cookedReadData) noexcept
{
try
{
auto& history = cookedReadData.History();
auto history = cookedReadData.History();

if (history.GetNumberOfCommands() <= 1 || _currentCommand == gsl::narrow<short>(history.GetNumberOfCommands()) - 1i16)
if (history->GetNumberOfCommands() <= 1 || _currentCommand == gsl::narrow<short>(history->GetNumberOfCommands()) - 1i16)
{
return STATUS_SUCCESS;
}
history.Swap(_currentCommand, _currentCommand + 1);
history->Swap(_currentCommand, _currentCommand + 1);
_update(1);
_drawList();
}
Expand All @@ -229,7 +229,7 @@ void CommandListPopup::_handleReturn(CookedRead& cookedReadData)
void CommandListPopup::_cycleSelectionToMatchingCommands(CookedRead& cookedReadData, const wchar_t wch)
{
short Index = 0;
if (cookedReadData.History().FindMatchingCommand({ &wch, 1 },
if (cookedReadData.History()->FindMatchingCommand({ &wch, 1 },
_currentCommand,
Index,
CommandHistory::MatchOptions::JustLooking))
Expand Down Expand Up @@ -346,7 +346,7 @@ void CommandListPopup::_drawList()
CommandNumberLength));

// write command to screen
auto command = _history.GetNth(i);
auto command = _history->GetNth(i);
lStringLength = command.size();
{
size_t lTmpStringLength = lStringLength;
Expand Down Expand Up @@ -423,13 +423,13 @@ void CommandListPopup::_update(const SHORT originalDelta, const bool wrap)
if (wrap)
{
// Modulo the number of commands to "circle" around if we went off the end.
NewCmdNum %= _history.GetNumberOfCommands();
NewCmdNum %= _history->GetNumberOfCommands();
}
else
{
if (NewCmdNum >= gsl::narrow<SHORT>(_history.GetNumberOfCommands()))
if (NewCmdNum >= gsl::narrow<SHORT>(_history->GetNumberOfCommands()))
{
NewCmdNum = gsl::narrow<SHORT>(_history.GetNumberOfCommands()) - 1i16;
NewCmdNum = gsl::narrow<SHORT>(_history->GetNumberOfCommands()) - 1i16;
}
else if (NewCmdNum < 0)
{
Expand All @@ -452,9 +452,9 @@ void CommandListPopup::_update(const SHORT originalDelta, const bool wrap)
else if (NewCmdNum > _bottomIndex)
{
_bottomIndex += delta;
if (_bottomIndex >= gsl::narrow<SHORT>(_history.GetNumberOfCommands()))
if (_bottomIndex >= gsl::narrow<SHORT>(_history->GetNumberOfCommands()))
{
_bottomIndex = gsl::narrow<SHORT>(_history.GetNumberOfCommands()) - 1i16;
_bottomIndex = gsl::narrow<SHORT>(_history->GetNumberOfCommands()) - 1i16;
}
Scroll = true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/host/CommandListPopup.hpp
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ Module Name:
class CommandListPopup : public Popup
{
public:
CommandListPopup(SCREEN_INFORMATION& screenInfo, const CommandHistory& history);
CommandListPopup(SCREEN_INFORMATION& screenInfo, const std::shared_ptr<CommandHistory>& history);

[[nodiscard]]
NTSTATUS Process(CookedRead& cookedReadData) noexcept override;
Expand All @@ -48,7 +48,7 @@ class CommandListPopup : public Popup

SHORT _currentCommand;
SHORT _bottomIndex; // number of command displayed on last line of popup
const CommandHistory& _history;
const std::shared_ptr<CommandHistory>& _history;

#ifdef UNIT_TESTING
friend class CommandListPopupTests;
Expand Down
2 changes: 1 addition & 1 deletion src/host/CommandNumberPopup.cpp
View file
Open in desktop
Expand Up @@ -102,7 +102,7 @@ void CommandNumberPopup::_handleEscape(CookedRead& cookedReadData) noexcept
void CommandNumberPopup::_handleReturn(CookedRead& cookedReadData) noexcept
{
const short commandNumber = gsl::narrow<short>(std::min(static_cast<size_t>(_parse()),
cookedReadData.History().GetNumberOfCommands() - 1));
cookedReadData.History()->GetNumberOfCommands() - 1));

CommandLine::Instance().EndAllPopups();
SetCurrentCommandLine(cookedReadData, commandNumber);
Expand Down
2 changes: 1 addition & 1 deletion src/host/CopyToCharPopup.cpp
View file
Open in desktop
Expand Up @@ -70,7 +70,7 @@ NTSTATUS CopyToCharPopup::Process(CookedRead& cookedReadData) noexcept
}

// copy up to specified char
const auto lastCommand = cookedReadData.History().GetLastCommand();
const auto lastCommand = cookedReadData.History()->GetLastCommand();
if (!lastCommand.empty())
{
_copyToChar(cookedReadData, lastCommand, wch);
Expand Down
12 changes: 6 additions & 6 deletions src/host/cmdline.cpp
View file
Open in desktop
Expand Up @@ -254,7 +254,7 @@ void SetCurrentCommandLine(CookedRead& cookedReadData, _In_ SHORT Index) // inde
NTSTATUS CommandLine::_startCommandListPopup(CookedRead& cookedReadData)
{
if (cookedReadData.HasHistory() &&
cookedReadData.History().GetNumberOfCommands())
cookedReadData.History()->GetNumberOfCommands())
{
try
{
Expand Down Expand Up @@ -336,7 +336,7 @@ NTSTATUS CommandLine::_startCopyToCharPopup(CookedRead& cookedReadData)
HRESULT CommandLine::StartCommandNumberPopup(CookedRead& cookedReadData)
{
if (cookedReadData.HasHistory() &&
cookedReadData.History().GetNumberOfCommands() &&
cookedReadData.History()->GetNumberOfCommands() &&
cookedReadData.ScreenInfo().GetBufferSize().Width() >= MINIMUM_COMMAND_PROMPT_SIZE + 2)
{
try
Expand Down Expand Up @@ -380,12 +380,12 @@ void CommandLine::_processHistoryCycling(CookedRead& cookedReadData,
return;
}
else if (searchDirection == CommandHistory::SearchDirection::Previous
&& cookedReadData.History().AtFirstCommand())
&& cookedReadData.History()->AtFirstCommand())
{
return;
}
else if (searchDirection == CommandHistory::SearchDirection::Next
&& cookedReadData.History().AtLastCommand())
&& cookedReadData.History()->AtLastCommand())
{
return;
}
Expand Down Expand Up @@ -543,8 +543,8 @@ void CommandLine::_deleteCommandHistory(CookedRead& cookedReadData) noexcept
{
if (cookedReadData.HasHistory())
{
cookedReadData.History().Empty();
cookedReadData.History().Flags |= CLE_ALLOCATED;
cookedReadData.History()->Empty();
cookedReadData.History()->Flags |= CLE_ALLOCATED;
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/host/cookedRead.cpp
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@
CookedRead::CookedRead(InputBuffer* const pInputBuffer,
INPUT_READ_HANDLE_DATA* const pInputReadHandleData,
SCREEN_INFORMATION& screenInfo,
CommandHistory* pCommandHistory,
std::shared_ptr<CommandHistory> pCommandHistory,
wchar_t* userBuffer,
const size_t cchUserBuffer,
const ULONG ctrlWakeupMask,
Expand Down Expand Up @@ -55,9 +55,9 @@ bool CookedRead::HasHistory() const noexcept
return _pCommandHistory != nullptr;
}

CommandHistory& CookedRead::History() noexcept
std::shared_ptr<CommandHistory> CookedRead::History() noexcept
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if this handed out a reference to the command history like it used to, it shouldn't be allowing other classes to share in the history ownership from here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

{
return *_pCommandHistory;
return _pCommandHistory;
}

void CookedRead::Erase()
Expand Down
6 changes: 3 additions & 3 deletions src/host/cookedRead.hpp
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@ class CookedRead final : public ReadData
CookedRead(InputBuffer* const pInputBuffer,
INPUT_READ_HANDLE_DATA* const pInputReadHandleData,
SCREEN_INFORMATION& screenInfo,
CommandHistory* pCommandHistory,
std::shared_ptr<CommandHistory> pCommandHistory,
wchar_t* userBuffer,
const size_t cchUserBuffer,
const ULONG ctrlWakeupMask,
Expand Down Expand Up @@ -69,7 +69,7 @@ class CookedRead final : public ReadData

SCREEN_INFORMATION& ScreenInfo();

CommandHistory& History() noexcept;
std::shared_ptr<CommandHistory> History() noexcept;
bool HasHistory() const noexcept;

void BufferInput(const wchar_t wch);
Expand Down Expand Up @@ -112,7 +112,7 @@ class CookedRead final : public ReadData
COORD _promptStartLocation;
// the location of the cursor before a popup is launched
COORD _beforePopupCursorPosition;
CommandHistory* _pCommandHistory; // non-ownership pointer
std::shared_ptr<CommandHistory> _pCommandHistory; // shared pointer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the comment on this line is redundant now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok...

// mask of control keys that if pressed will end the cooked read early
const ULONG _ctrlWakeupMask;
// current state of the CookedRead
Expand Down