Move free functions definitions to GncOptimizer.cpp#2461
Move free functions definitions to GncOptimizer.cpp#2461varunagrawal merged 8 commits intodevelopfrom
GncOptimizer.cpp#2461Conversation
| namespace gtsam { | ||
|
|
||
| /* ************************************************************************* */ | ||
| bool isNullType(Type type) { return type == Type::NullPointer; } |
There was a problem hiding this comment.
@dellaert I believe Type is too generic an enum name, and it should instead be GncFactorType to better describe its purpose. Should I add that change to this PR?
There was a problem hiding this comment.
I agree. Could we make it an internal enum so it's GncOptimizer::Type?
There was a problem hiding this comment.
That would involve providing the template parameters, making the type name quite verbose, e.g. GncOptimizer<GncParams<LevenbergMarquardtParams>>::FactorType.
I am using FactorType since GncOptimizer::Type reads like it is a type variant of GncOptimizer and not the factors under consideration.
There was a problem hiding this comment.
Additionally, all the functions would now have to be templated. Would you prefer we move them inside the GncOptimizer class definition instead of keeping them as free functions?
There was a problem hiding this comment.
Okay making the internal namespace is a significant refactor which I would prefer to do after a design review. I'll change the name to GncFactorType and we can revisit this in the future.
dellaert
left a comment
dellaert
left a comment
There was a problem hiding this comment.
This solution is fine with me.
2 participants
Added a new file called
GncOptimizer.cppso that I could move the free function definitions to it. These were originally added and defined erroneously in #2410.This resolves the duplicate symbol error reported in #2460.