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

Convert CommandHistory to a std::shared_ptr #1161

Open
wants to merge 5 commits into
base: dev/austdi/NewCookedRead
from

Conversation

Projects
None yet
4 participants
@ChristophePichaud
Copy link

commented Jun 7, 2019

Summary of the Pull Request

CommandHistory becomes a std::shared_ptr. Minor changes in code and unit tests.
Some ctor has been modified.
There is not weak_ptr usage or raw pointer. Every times a shared_ptr is passed, its using the ref counter of the = operator. It prevents unexpected behaviour and hangs.

References

My previous PR 1148 was bad quality. you can cancel it.

PR Checklist

  • Closes #1058
    [X] Cancel PR #1148
    [X] CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
    [X ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Previous commit was a mistake, it involves modifcations and both the migration to vs2019 and platform toolset v142.
The new PR is juts handling H/CPP modifications.
Worked in VS2017 OK.

Validation Steps Performed

Manuel test of running tab and ask for cmd and type command, and ask for history...

@ChristophePichaud ChristophePichaud referenced this pull request Jun 7, 2019

Closed

Dev/austdi/new cooked read #1148

3 of 5 tasks complete
@@ -22,7 +22,7 @@ static constexpr size_t COMMAND_NUMBER_SIZE = 8; // size of command number buf
// - history - the history to look through to measure command sizes
// Return Value:
// - the proposed size of the popup with the history list taken into account
static COORD calculatePopupSize(const CommandHistory& history)
static COORD calculatePopupSize(const std::shared_ptr<CommandHistory>& history)

This comment has been minimized.

Copy link
@dlong11

dlong11 Jun 7, 2019

In this case, the calculatePopupSize can be left as a const reference.

R.30: Take smart pointers as parameters only to explicitly express lifetime semantics


return { gsl::narrow<short>(width), height };
}

CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const CommandHistory& history) :
CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const std::shared_ptr<CommandHistory>& history) :

This comment has been minimized.

Copy link
@dlong11

dlong11 Jun 7, 2019

@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.

This comment has been minimized.

Copy link
@ChristophePichaud

ChristophePichaud Jun 7, 2019

Author

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.

This comment has been minimized.

Copy link
@ChristophePichaud

ChristophePichaud Jun 7, 2019

Author

I have applied the guidance. It's cool.

This comment has been minimized.

Copy link
@dlong11

dlong11 Jun 8, 2019

@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?

This comment has been minimized.

Copy link
@dlong11

dlong11 Jun 8, 2019

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.

This comment has been minimized.

Copy link
@ChristophePichaud

ChristophePichaud Jun 9, 2019

Author

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 ?

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 10, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ChristophePichaud

ChristophePichaud Jun 10, 2019

Author

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.

@zadjii-msft zadjii-msft changed the title Changes for Issue #1058 are done in VS2017. Works fine. Convert CommandHistory to a std::shared_ptr Jun 7, 2019

@zadjii-msft zadjii-msft requested a review from adiviness Jun 7, 2019

@zadjii-msft

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Since this is a PR into @adiviness's branch, I'm gonna defer to his judgement

@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

I have transformed the 'static COORD calculatePopupSize(const CommandHistory& history)' to take back a reference. The caller is simple:
'CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const std::shared_ptr& history) :
Popup(screenInfo, calculatePopupSize(*history)),
_history{ history },' ../..

I think its a good balanced between shared_ptr and reference.
Because the history is kept in a list, it's real shared_ptr stuff.
I am confident with the code. Tell me back !


return { gsl::narrow<short>(width), height };
}

CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const CommandHistory& history) :
CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const std::shared_ptr<CommandHistory>& history) :

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 10, 2019

Contributor

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.

@@ -55,9 +55,9 @@ bool CookedRead::HasHistory() const noexcept
return _pCommandHistory != nullptr;
}

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

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 10, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ChristophePichaud
@@ -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

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 10, 2019

Contributor

nit: the comment on this line is redundant now.

This comment has been minimized.

Copy link
@ChristophePichaud
Christophe Pichaud
@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

Changes done.

@@ -50,7 +50,7 @@ static COORD calculatePopupSize(const CommandHistory& history)
return { gsl::narrow<short>(width), height };
}

CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, const CommandHistory& history) :
CommandListPopup::CommandListPopup(SCREEN_INFORMATION& screenInfo, CommandHistory& history) :

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 11, 2019

Contributor

This is missing the const it had previously

This comment has been minimized.

Copy link
@ChristophePichaud
@@ -21,7 +21,7 @@ Module Name:
class CommandListPopup : public Popup
{
public:
CommandListPopup(SCREEN_INFORMATION& screenInfo, const CommandHistory& history);
CommandListPopup(SCREEN_INFORMATION& screenInfo, CommandHistory& history);

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 11, 2019

Contributor

Another missing const

This comment has been minimized.

Copy link
@ChristophePichaud
@@ -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; // CommandHistory pointer

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 11, 2019

Contributor

I'd just remove the comment on this line totally, it serves no purpose anymore.

This comment has been minimized.

Copy link
@ChristophePichaud
Christophe Pichaud
Add missing const stuff.
Remove a comment.
@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

Yes, it was my fault... two missing const. I have to recognize I could have found it sooner...

@@ -151,7 +151,7 @@ NTSTATUS CommandListPopup::_deleteSelection(CookedRead& cookedReadData) noexcept
{
try
{
auto& history = cookedReadData.History();
auto history = cookedReadData.History();

This comment has been minimized.

Copy link
@adiviness

adiviness Jun 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@ChristophePichaud
@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

Modifications done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.