Refactor measurement containers #123
Conversation
leokoppel
force-pushed
the
refactor_measurement_containers
branch
from
5bf290b to
7ab206a
Compare
navganti
left a comment
navganti
left a comment
There was a problem hiding this comment.
I'll be honest, I'm struggling a bit with the TMP. Even more so now that it's recursive...
I'm going to take another look at it a bit later. Haven't noticed anything major yet; just have a few questions.
There was a problem hiding this comment.
Do you still need this? I see you're using T for the rest of this struct
There was a problem hiding this comment.
Yes. T does not persist outside of this definition. MeasurementType is defined so it can be used by MeasurementContainerBase, from outside of this definition.
There was a problem hiding this comment.
Ah, right...because Base is templated with either the landmark or regular measurement containers.
I was actually wondering about where MeasurementType was defined in the base class; that makes a bit more sense now. Thanks
There was a problem hiding this comment.
You have two sets of protected: specified objects. Should they be consolidated?
There was a problem hiding this comment.
Hmm maybe. I sorted them by types and data members but I actually have a typedef down here.. good catch
There was a problem hiding this comment.
Removed the typedef at the bottom (and the protected at the top)
There was a problem hiding this comment.
Why do you need to specify a local variable here?
There was a problem hiding this comment.
So the next line doesn't go over 80 characters ; )
But also, it makes it clear I just use the result of one call composite(); that it doesn't change or have side effects.
There was a problem hiding this comment.
Okay, sounds good. Was just curious since you were just directly modifying this->composite all throughout the rest of those similar methods.
The goal is to reduce code duplication between the similar MeasurementContainer and LandmarkMeasurementContainer (and any other future variations). To do this, use CRTP and refine the "internal::measurement_container" struct as a traits class. Only modify MeasurementContainer in this commit; LandmarkMeasurementContainer is next. Resources: - http://eli.thegreenplace.net/2011/05/17/the-curiously-recurring-template-pattern-in-c - https://stackoverflow.com/questions/4173254/what-is-the-curiously-recurring-template-pattern-crtp - https://stackoverflow.com/questions/6006614/c-static-polymorphism-crtp-and-using-typedefs-from-derived-classes
The problem was the test getAllFromSensor for getAllFromSensor was failing because the results were not ordered by time. Because MeasurementContainerBase<T>::getAllFromSensor() uses the traits type "sensor_index", make LandmarkMeasurementContainer's "sensor_index" include ordering by time. Do this by simply renaming its "sensor_composite_index".
Clearly differentiate: * Derived: the type of derived measurement container * T: the type of measurement
There is no reason for this change except it fixes a resolution problem in my IDE. There is no real reason to avoid the change either.
leokoppel
force-pushed
the
refactor_measurement_containers
branch
2 times, most recently
from
39afe88 to
3794745
Compare
The class definition is organized, top to bottom: - typedefs - methods - data members The first typedef is private, and is first because it is used for convenience of defining the other types. Remove an unneeded typedef used only in the subsequent two lines. Change "protected" traits typedef to private, and redefine it in derived classes to avoid possibly confusing "Base::traits".
leokoppel
force-pushed
the
refactor_measurement_containers
branch
from
3794745 to
036f03d
Compare
Further to #41. Code duplication was noted in PR #82.
Reduce code duplication between the similar MeasurementContainer and LandmarkMeasurementContainer (and any other future variations).
This is done through static polymorphism, using CRTP (the technique used extensively by Eigen). References:
Pre-Merge Checklist:
clang-formatTesting done: existing unit tests. There is no behaviour change, so unit tests are untouched.