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

Replace use of raw deletable pointers in readDataCooked with shared/weak ptrs #1058

Open
ChristophePichaud opened this issue May 30, 2019 · 9 comments

Comments

Projects
None yet
5 participants
@ChristophePichaud
Copy link

commented May 30, 2019

Summary of the new feature/enhancement

In Module Name (readDataCooked.hpp), there is a // TODO MSFT
// TODO MSFT:11285829 make this something other than a deletable pointer
// non-ownership pointer
CommandHistory* _commandHistory;

Proposed technical implementation details (optional)

In project Host, I would like to encapsulate the _commandHistory raw pointer to CommandHistory as a std::shared_ptr.
I am learning the Host project and I think I can do it.

The main change propagation problem is that:

CommandHistory& COOKED_READ_DATA::History() noexcept { return *_commandHistory; }

Should I transform it as a :
std::shared_ptr<CommandHistory> COOKED_READ_DATA::History() noexcept { return _commandHistory; // assume it's a shared_ptr<> }

or should I return a weak_ptr... I would be dirty ! What is your opinion ? The History() fn is used to get the reference and some function use the reference. It's not a problem to change the reference as a shared_ptr, its just the fact I need to make more job. The idea is to be clean so I would prefer to propagate shared_ptr. For a ISO C++, it's better. weak_ptr should be used for legacy stuff thunking layers stuff but for new C++ project, unique_ptr<> & shared_ptr<> everywhere is the rule.

Tell me and I can do it.

There are changes to propagate because the pointer is used as that (Find In Files):
Code File Line Column
_commandHistory{ CommandHistory }, D:\Dev\Terminal\src\host\readDataCooked.cpp 64 5
return _commandHistory != nullptr; D:\Dev\Terminal\src\host\readDataCooked.cpp 142 12
return _commandHistory; D:\Dev\Terminal\src\host\readDataCooked.cpp 147 13
if (_commandHistory) D:\Dev\Terminal\src\host\readDataCooked.cpp 387 9
if (_commandHistory) D:\Dev\Terminal\src\host\readDataCooked.cpp 1025 17
LOG_IF_FAILED(_commandHistory->Add({ _backupLimit, StringLength / sizeof(wchar_t) }, D:\Dev\Terminal\src\host\readDataCooked.cpp 1029 31
CommandHistory
_commandHistory; D:\Dev\Terminal\src\host\readDataCooked.hpp 142 21
cookedReadData._commandHistory = pHistory; D:\Dev\Terminal\src\host\ut_host\CommandLineTests.cpp 81 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 113 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 157 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 198 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 239 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 277 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 315 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 355 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 386 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 417 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 459 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 498 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandListPopupTests.cpp 543 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandNumberPopupTests.cpp 92 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandNumberPopupTests.cpp 137 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandNumberPopupTests.cpp 166 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandNumberPopupTests.cpp 214 28
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CommandNumberPopupTests.cpp 260 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CopyToCharPopupTests.cpp 91 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CopyToCharPopupTests.cpp 126 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CopyToCharPopupTests.cpp 157 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CopyToCharPopupTests.cpp 197 24
cookedReadData._commandHistory = m_pHistory; D:\Dev\Terminal\src\host\ut_host\CopyToCharPopupTests.cpp 238 24

@miniksa

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I haven't heard the guidance before that std::weak_ptr should only be used as a kludge for legacy code. Do you have a link to a reference to support that?

Personally, I think this might be an ideal std::weak_ptr scenario as a cursory look at the code suggests that the usages of the ::History ref are also simultaneously checking the boolean value of whether there is any history. That leads me to believe that if it were a std::weak_ptr and it's not promotable to std::shared_ptr at the time of usage... then it's fine... there's no history and we can skip those blocks (without also needing the boolean check.)

I can be convinced otherwise as well. I'm honestly not super familiar with shared_ptr and weak_ptr rules and semantics yet (as evidenced by how few of them we have in this codebase to date).

Finally, I think that in order to accomplish this successfully, you will likely have to change the way that history.cpp is handing out the pointers in the first place with the s_Find method. Given that the underlying CommandHistory elements are stored in a list straight up (and based on the comments, they're subject to getting re-ordered by the other methods in the class), this might have to change to a different storage structure that holds a bunch of shared_ptrs or something like that.

@DHowett-MSFT DHowett-MSFT changed the title Technical Request - TODO MSFT:11285829 Replace use of raw deletable pointers in readDataCooked with shared/weak ptrs May 30, 2019

@DHowett-MSFT

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Renamed this to cover what it really is. Thanks for reporting!

@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented May 31, 2019

Question ? I am new to contribution so here is my question... Can I start working on my local branch or should we continue the discussion about how I will do about impact & more ?
In my opinion, I have everything to start the job but I prefer to follow the process and ask.
It's up to you to tell me...

@zadjii-msft

This comment has been minimized.

Copy link
Member

commented May 31, 2019

@ChristophePichaud I'd make sure to coordinate with @adiviness. He's been working on some pretty extensive overhauls in this area. I'm not the expert here, but I'm pretty sure any work here will be pretty heavily conflicting with that.

@adiviness

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Yeah, my branch dev/austdi/NewCookedRead is a pretty large overhaul of the COOKED_READ_DATA class and in general a lot of the command line interactions that utilize the cooked read class. We might want to do something like PR any changes you make into that branch instead of master to cut down on merge conflicts later, but it's not something we as a team have encountered or really thought about yet process-wise.

shared_ptr I assume would be what we would want to use, if we don't have a history for the cooked read then it would be an empty smart pointer that we could check against.

On the contribution side, in order to reduce conflicts of effort we encourage people to open up a github issue to chat with us before making code changes. This scenario where I have a branch with large changes in the related area is a good example of that. So we definitely appreciate you reaching out to us first! What I would encourage is for you to take a look at the branch I mentioned above so you can see the way we envision the code in that area evolving and if you believe you can convert the history pointer to a smart pointer without too much code conflicts then you're more than welcome to try. And of course we'll be around to help with questions and whatnot.

@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 2, 2019

@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

Hello @adiviness, I have made the various shared_ptr propagation, and -> and (*it) transformation in host and unit tests on my fork of your branch 'dev/austdi/NewCookedRead'.
It implies some ctor changes but not so many changes. It's plumbing.
I'm doing github commands...

@ChristophePichaud

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@adiviness, could you give more guidance about running the units tests ? It seems I need to install Google tests or it's in vs2019 bundle ? I have forgotten that... Do you have a readme.md page for unit tests ?

@ChristophePichaud ChristophePichaud referenced this issue Jun 5, 2019

Closed

Dev/austdi/new cooked read #1148

3 of 5 tasks complete
@adiviness

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

We use TAEF for our tests, I believe it's a nuget package in /dep/packages. I'm not sure how you would run them in Visual Studio though. /doc/building.md explains how to build and run the tests with either powershell or cmd if Visual Studio continues to be troublesome.

ChristophePichaud pushed a commit to ChristophePichaud/terminal that referenced this issue Jun 7, 2019

adiviness added a commit that referenced this issue Jun 19, 2019

Convert CommandHistory to a std::shared_ptr (#1161)
* Changes for Issue #1058 are done in VS2017. Works fine.

* calculatePopupSize use a const CommandHistory& and is called from a shared_ptr.

* Some shared_ptr are converted into refrences.
Better appropriated.

* Add missing const stuff.
Remove a comment.

* cookedReadData.History() returns a ref. Callers adapted. OK.
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.