0

I want to do this project in C++ and be done with it once and for all: select a movie from a list in one window and display the movie's details in another window.

I'd already arrived at Remy Lebeau's solution here and found his post because I'd realized the same limitation he did: it exposes the raw pointer to memory leakage.

(Please excuse my coding style).

MoviesWindow::MovieSelected( const unsigned int uint__MovieKey )
{   ...
    std::unique_ptr<MovieBean> uptr__MovieBean = std::make_unique...
    ...
    uptr__MovieBean->SetMovieTitle(   row["MovieTitle"]   );
    uptr__MovieBean->SetYearReleased( row["YearReleased"] );
    ...
    SendMessage( hwnd__MovieWindow, UWM_SendMovieBean, 0, (LPARAM) uptr__MovieBean->get() );
    ...
}

Fortunately, I have an advantage with SendMessage(): it won't end (releasing the std::unique_ptr) until the message handler returns, and the message handler's job is to clone the MovieBean.

MovieWindow::HandleUwmSendMovieBean( const HWND hwnd__MovieWindow, const UINT uint__WindowMessage, const WPARAM wParam, const LPARAM lParam )
{   UPTR__MovieBean = std::move( (*((MovieBean*) lParam).Clone() );
    ...
}

It seems the right way to do it is leave ownership of the std::unique_ptr with MovieSelected() until the actual moment of transfer by sending a reference to the handler, and using std::move on the reference.

MoviesWindow::MovieSelected( const unsigned int uint__MovieKey )
{   ...
    SendMessage( hwnd__MovieWindow, UWM_SendMovieBean, 0, (LPARAM) &uptr__MovieBean );
    ...
}

But I cannot figure out how to get the reference out of lParam and into std::move. This would seem to be the one, but nope.

MovieWindow::HandleUwmSendMovieBean( const HWND hwnd__MovieWindow, const UINT uint__WindowMessage, const WPARAM wParam, const LPARAM lParam )
{ UPTR__NewMovieBean  = std::move( reinterpret_cast<std::unique_ptr<MovieBean>*>( lParam ) );
    ...
}

I've tried every combination I can think of and ensured that the address sent is the address received and supplied to std::move. All I get are constant compiler errors (including a now-much-hated one about std::remove_reference), and crashes. Any insight would be appreciated.

pbyhistorian
  • 341
  • 2
  • 10
  • `std::move` in `std::move(movieBean.Clone())` or `std::move(somePointer)` is useless. For the former, `Clone()` (should) already returns by value, for the later, moving pointer is the same as copy. – Jarod42 May 28 '20 at 10:22
  • 1
    You should not use double-underscore in your own idnetifiers, they are reserved for impementation use – M.M Jun 01 '20 at 20:45
  • @Jarod42 My idea was to protect ownership of the `MovieBean` while it's being passed through a middleman - `SendMessage()`. `std::move()` completes the transfer of ownership from one `std::unique_ptr` to the other. Assigning a pointer to `UPTR__NewMovieBean` fails to compile because the `std::unique_ptr` copy constructor is (as it should be) deleted. I believe the same would be true for the `std::unique_ptr` returned by MovieBean.Clone(). – pbyhistorian Jun 03 '20 at 17:57

2 Answers2

2

SendMessage and unique_ptr are definitely not made for each other. I can give lots of hints here, but the best thing to do is to not be smuggling smart pointers through HWND messages.

I don't know what a UWM_SendMovieBean, but I'm guessing that's a custom windows message you either based of off WM_USER or RegisterWindowsMessage. So what you are really doing sounds like you are trying to signal another code component with SendMessage instead of a formal contract between the backing classes of both windows. It's not the worst thing ever to do this, but with a class as specialized as unique_ptr, it gets much harder to pull off.

The cheap and easy thing to do would be to have both windows share the row data structure that you have. Then your send message simply sends the uint__MovieKey as the WPARAM or LPARAM of your custom message.

But if you were on my team, and we were building this app together, I'd give you the crash course in Model-View-Presenter (and other MVC designs) that make these types of feature designs more maintainable.

selbie
  • 100,020
  • 15
  • 103
  • 173
  • Oddly, "smuggling" the unique_ptr was an attempt to use what rudimentary MVP I still remember. The obvious option I overlooked was the "formal contract between the backing classes", which might let me replace my registered window message with something simple, like UpdateWindow(). Still, smuggling the unique_ptr seems kinda cool, and something the C++ designers might approve of: protecting objects even in flight between windows. – pbyhistorian May 28 '20 at 15:32
  • 2
    I was trying to tell you, in the politest sense possible, that what you are proposing is bad design. Consider how your list window has the HWND of the detail window. However that HWND was passed to the list window in the first place, there's certainly a way to pass a C++ class pointer instead so that methods can be formally invoked instead of trying to do it through a SendMessage call. – selbie May 28 '20 at 15:51
  • Yes, that's what I'm doing now. It's a much cleaner approach and I'm already set up for it. Thank you for pointing me where I should have been. But I'm still wondering why SendMessage() is a bad design. It's just passing a unique_ptr by reference (which is allowed for corner cases) in a rather difficult way. And "smuggling" made it sound even better. – pbyhistorian May 28 '20 at 19:10
1

I got the answer in two more tries:

MovieWindow::HandleUwmSendMovieBean( const HWND hwnd__MovieWindow, const UINT uint__WindowMessage, const WPARAM wParam, const LPARAM lParam )
{ UPTR__NewMovieBean = std::move( *((std::unique_ptr<MovieBean>*) lParam) );
  ...
}

Basically, sending the address of the std::unique_ptr<MovieBean> meant I was sending a pointer, so I needed to std::move(...) what the pointer was pointing to. It works fine.

This still seems sound to me: I'm leaving the MovieBean protected by a smart pointer while it's "in flight". It's effectively just a pass-by-reference that doesn't violate C++ Core Rule 33: Take a unique_ptr<widget>& parameter to express that a function reseats the widget.

The substantial problem in this usage is that my message handler is also the signal to read the MovieBean's copious information into the various CommonControls of the MovieWindow. Message handlers are supposed to be quick. The better approach is what @selbie suggested: the formal contract between the backing classes.

pbyhistorian
  • 341
  • 2
  • 10