Why change from ScalarTransformer to Transformer? Ideally, whatever the reason, ScalarTransformer should be 'fixed' / 'augmented' / ... to support this use-case...
Summary of the discussion:
Leave problem in Track v1, fixing it might introduce other headaches
Drop dependency on KeyedObject in (upcoming) Track v2.
Specify move constructors noexcept so that they can be used when resizing vectors.
0 of 1 checklist item completed
· Edited
Designs
Child items
...
Show closed items
Linked items
0
Link issues together to show that they're related or that one is blocking others.
Learn more.
I tried to look at this especially because in some places keys are not preserved. E.g. the PrPixelTracking assigns a key but this key can be invalidated when the output container (now a std::vector) is resized. However I am not sure how to preserve the key, a KeyedObject has a private copy constructor and no move constructor. The constructor which takes a key_type, automatically sets hasKey to true which is also unwanted.
Note that at some point (rather sooner than later!) we should drop the KeyedObject inheritance from Track.
Coming back to the key, I'd think that when a track is moved the key should be moved with it (i.e. preserved), and when it is copied, a new key should be generated. The reason is that the key is used to 'identify' a track, and moving a track doesn't change its identity. And when copying, the new track (which happens to have the same 'content') should have a new key, as no two tracks should co-exist with the same key.
(note that (some of) the constraints on keys come from an era that 'move' was impossible to define in code)
To answer your question: I see no reason why that would not work (double negative ;-)
Note that at some point (rather sooner than later!) we should drop the KeyedObject inheritance from Track.
We could do that in the Track v2.
Coming back to the key, I'd think that when a track is moved the key should be moved with it (i.e. preserved), and when it is copied, a new key should be generated. The reason is that the key is used to 'identify' a track, and moving a track doesn't change its identity. And when copying, the new track (which happens to have the same 'content') should have a new key, as no two tracks should co-exist with the same key.
I guess then ContainedObject needs a default move constructor as well.
Actually, ContainedObject has a more fundamental problem -- it tries to answer a question, which I think it does not have enough information about to ever answer... What does it mean to be 'contained'? This class defines that as 'being part of an container which derives from ObjectContainerBase` -- now, if one moves a contained object is the moved object also part of the container? The object itself can just not know this -- when sorting a ObjectContainerBase, I'm moving objects inside the container to a different location in the container -- but I could also move object out of the container.
Now, this is not unique to moving, but also copying -- and the copy constructor says that a copy is NOT in the same container (presumably then demanding the code that makes the copy to 'fix up' that answer in case it is not appropriate). And since in C++, in the absence of a move constructor, move implies copy (think 'what does int do?', i.e. what does an 'int' do when it is moved -- answer: it copies the value). So in this case, I'd say that 'move' should behave like copy -- if copy is allowed -- unless there is a reason it shouldn't.
So while 'KeyedObject' answers the question 'who am I', 'ContainedObject' has to answer 'where am I' -- something that is difficult to do when its basically sensory deprived of information about the rest of the code.
Hence the question: do we really need ContainedObject here? What purpose does it serve? Its use seems to be that one bit of code grabs an object from a container, passes it along, and then some bit of code later that code wants to know in which container the object resides -- what is the real use case for that, and is that the only way to achieve the required result? (i.e. yes, I know of code that does this, related to persisting, as it wants to persist containers instead of individual objects -- think of having a container of vertices, which refer to particles, and when persisting the container of vertices, the code finds the set of containers of particles which also need to be persisted -- but is there not another way to achieve the required result?)
my thinking is, what can a.key() return if a.hasKey() == false? Basically a special value indicating 'no key', and I would presume that that is exactly what Key{} would produce...
Basically, I think that as the code has a bool + value setup, it does implicitly what std::optional<Key> would do explicitly, and in modern C++, the code would have been written using std::optional<Key> instead. But as the bool and the Key are actually seperate, one has to consider what is the value of Key when hasKey is false -- and the answer seems to be: default constructed. (at which point one can wonder why bother with the bool, as then hasKey could be implemented as a.key() != Key{} -- i.e. Key does not need all the bits it has to represent all valid keys -- think equivalent of NaN for floating point numbers.
Absolutely, I am definitely fighting a symptom and not the cause. It looks like the interface of KeyedObject in case we need some form of it in the future can be improved.
Also, another 'weird' think in the design is that KeyedObjectinherits from ContainedObject -- whereas the concept of a 'key' (identifier) is more generic, so I see no reason that anything with a key must be 'contained'. And then, KeyedObject also implements reference counted lifetime management -- which is again a different concept. So KeyedObject is actually mixing three things together which IMHO should not be combined...
I think that KeyedObjectshould have a move constructor from first principles (or more accurately: I cannot think of a reason, from first principles, why it should not or could not have one, so ergo it should).
So I'd start by adding it, and then worry about the subsequent fall-out...
Okay, I tried specifying default move and operator= in KeyedObject. Then I was surprised that it still does not work. Apparently GOD also needs the feature of noexcept. The move constructor (of Track) needs to be declared noexcept to be called when resizing a vector.