[RST-2427] Added a 'source' field to the constraints #101
Conversation
…ange and hence touches a lot of files.
svwilliams
changed the title
svwilliams
changed the title
There was a problem hiding this comment.
LGTM, but it seems to me that you're missing direct initialization (https://en.cppreference.com/w/cpp/language/direct_initialization) with list initialization (https://en.cppreference.com/w/cpp/language/list_initialization), or maybe aggregate initialization (https://en.cppreference.com/w/cpp/language/aggregate_initialization) 🤔
| const Variable& variable, | ||
| const fuse_core::VectorXd& mean, | ||
| const fuse_core::MatrixXd& covariance) : | ||
| fuse_core::Constraint{variable.uuid()}, | ||
| fuse_core::Constraint(source, {variable.uuid()}), // NOLINT(whitespace/braces) |
There was a problem hiding this comment.
I think this should also work: fuse_core::Constraint{source, variable.uuid()}, (same in other instances below)
There was a problem hiding this comment.
That particular incantation results in:
error: no matching function for call to ‘std::vector<boost::uuids::uuid>::vector(boost::uuids::uuid&)’
This does work, but I'm not sure it has any advantages over the constructor () version:
fuse_core::Constraint{source, {variable.uuid()}}
I could do something with variadic templates:
https://stackoverflow.com/questions/25721869/initializer-list-combined-with-other-parameters
public:
template <class ...Args>
Constraint(const std::string& source, Args... args) : Constraint(source, {args...}) {}
private:
Constraint(const std::string& source, std::initializer_list<UUID> variable_uuid_list) :
source_(source),
variables_(variable_uuid_list)
{}
but I'm not sure it is worth the trouble.
There was a problem hiding this comment.
Fair enough. I don't think it's worth the trouble, and I didn't realized there's an std::initializer_list for the UUIDs.
| const Variable& variable1, | ||
| const Variable& variable2, | ||
| const fuse_core::VectorXd& delta, | ||
| const fuse_core::MatrixXd& covariance) : | ||
| fuse_core::Constraint{variable1.uuid(), variable2.uuid()}, | ||
| fuse_core::Constraint(source, {variable1.uuid(), variable2.uuid()}), // NOLINT(whitespace/braces) |
There was a problem hiding this comment.
As mentioned before, I think this should work: fuse_core::Constraint{source, variable1.uuid(), variable2.uuid()}, (same below)
| const fuse_variables::Orientation3DStamped& orientation, | ||
| const fuse_core::Vector4d& mean, | ||
| const fuse_core::Matrix3d& covariance) : | ||
| fuse_core::Constraint{orientation.uuid()}, | ||
| fuse_core::Constraint(source, {orientation.uuid()}), // NOLINT(whitespace/braces) |
There was a problem hiding this comment.
fuse_core::Constraint{source, orientation.uuid()},
Added a 'source' field to the constraints. This is an API-breaking change and hence touches a lot of files.