stewartmatheson / projectx

0 stars 1 forks source link

Move `ViewLayer::views` to `std::unique_ptr` #53

Closed msbit closed 3 years ago

msbit commented 3 years ago

Move ViewLayer::views to std::vector<std::unique_ptr<>> and update ViewLayer::AddView to both take an r-value, and ensure a move into the std::vector with std::move. This matches all the current call-sites, as they are all passed temporaries.

msbit commented 3 years ago

I'm guessing that throughout the big refactoring that you finished up, you will have hit two things:

  1. you can't have a heterogeneous std::vector with subclass instances (eg none of BoxSelectionView EntityView GridSelectionView GridView SelectedTileView TileBackgroundView TilePaletteView ToolbarToolsView can go into a std::vector<View<HouseSceneState>> so you need a pointer (or preferably smart pointer)
  2. you would have preferred a std::vector<std::unique_ptr<View<HouseSceneState>>> but were getting obtuse compilation errors like:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1881:31: error: call to implicitly-deleted copy
      constructor of 'std::__1::unique_ptr<View<HouseSceneState>, std::__1::default_delete<View<HouseSceneState> > >'
            ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
                              ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:2518:3: note: copy constructor is implicitly
      deleted because 'unique_ptr<View<HouseSceneState>, std::__1::default_delete<View<HouseSceneState> > >' has a user-declared move constructor
  unique_ptr(unique_ptr&& __u) _NOEXCEPT
  ^

Basically, this is the way that std::unique_ptr enforces its uniqueness; the copy constructor doesn't exist!

What we can do, instead, is accept an r-value reference, making the AddView method:

void AddView(std::unique_ptr<View<HouseSceneState>> &&)

which opens up the possibility of moving r-values into the function, then pass this on to views.push_back with std::move. Basically what this does is "move" the std::unique_ptr created at the call-site of ViewLayer::AddView, which leaves it in a very restricted state (only able to be destructed I think?) but that's fine because they are temporaries and would almost immediately die on the same line they are created.

This covers it pretty well.

Think about it for a bit, r-values are a bit of a departure, but them and move semantics allow some pretty nifty optimisations that copy constructors really can't.

stewartmatheson commented 3 years ago

Right.......

This has been a bit of a source of confusion for me for a while. Your comment explains it really well. The SO answer is good too. I did hit that exact issue you highlighted in your above post. I have never used smart pointers before this code but I totally get it now. In a way comes back to understanding who "owns" what memory. I did change to shared pointers from unique pointers for that reason.

msbit commented 3 years ago

In a way comes back to understanding who "owns" what memory

Pretty much. Provided a good smart pointer implementation, it brings the stack semantics to something that lives on the heap, as the destructor of the smart pointer will be called in a well-defined way when it goes out of scope. You can definitely still leak the contained pointer with std::unique_ptr::get, but that kind of usage will stick out like a sore thumb :)