search

envire / envire-envire_core

Core part for the Environment Representation library
BSD 2-Clause "Simplified" License
7 stars 13 forks source link

add clone function for templated items #30

Closed planthaber closed 6 years ago

planthaber commented 6 years ago

Uses the copy constructor of the tempate class to create a deep copy of the item base pointer

I have to put ItemBase::Ptr in a queue to be handled, meanwhile the original item in the graph might be changed

If anyone implements the ItemBase directly, whithout using the templated Item.hpp, this might break some stuff because clone() is pure virtual in ItemBase

chhtz commented 6 years ago

I'm ok with this patch. Having two bool arguments might be a source of error, though. Is it more likely that you want to keep the frame and or the id or not? (or just the id, but not the frame?)

Unrelated: Can't all copy/move constructors/assignments be =default; here?

planthaber commented 6 years ago

Hmm, no idea, we could make clone keep ID and Frame and then the user has to overwrite them in case he wants a clone with other ID.

In my case, they have to stay the same anyways

chhtz commented 6 years ago

If staying the same is your use case (which at the moment likely is the only use case), I'd suggest removing the arguments, and just return ItemBase::Ptr(new Item(*this)); (untested, but I don't see what could go wrong).

planthaber commented 6 years ago

Item(*this) does not initialize the spatiotemporal object with its data, thus requiring derived classes (to be saved inside the spatiotemporal) to have an standard constructor Derived() without any parameters (templates are fun...). This already breaks the test cases.

I'll just remove the params from clone(), but stay with new Item<_ItemData>(this->getData())