Fix deleting an element from OpenSim::Coordinate's causes getMinRange… #3546
Conversation
| // eagerly check if outside code has somehow managed to remove elements | ||
| // from this coordinate's `range` property (issue #3532) | ||
| OPENSIM_THROW_IF( | ||
| getProperty_range().size() < 2, |
There was a problem hiding this comment.
Fewer than 2 elements doesn't make sense for a range, but neither does more than 2. Perhaps demand that the size is equal to 2?
There was a problem hiding this comment.
The size might be enforced automatically via the property macro that defines it, OpenSim_DECLARE_LIST_PROPERTY_SIZE.
There was a problem hiding this comment.
The size might be enforced automatically via the property macro that defines it,
OpenSim_DECLARE_LIST_PROPERTY_SIZE.
The macro pumps the size to constructProperty_##name, which moves it into AbstractProperty::_minListSize etc, which will then range-check:
AbstractProperty::setValueAbstractProperty::removeValueAtIndex
But I don't think it will currently range-check:
AbstractProperty::clearoperator=(PropertyWithNoRangeLimits const&)AbstractProperty::appendValueAbstractProperty::assign
With clear being the one that the test exercises, but it's also sometimes necessary (e.g. in property editors) to do stuff like create a new property, only copy over the elements the user wants, and then assign the new property over the old one, which will delete the range check :<
There was a problem hiding this comment.
I think OpenSim_DECLARE_LIST_PROPERTY_SIZE should maybe have more enforcement, but that's a wider change than this specific issue.
Apart from that, I have changed the check to be == 2, rather than >= 2, if that resolves this conversation?
Fixes issue #3532
Brief summary of changes
OpenSim::Coordinate::extendFinalizeFromPropertiesTesting I've completed
OpenSim::Coordinate'srangeproperty causesgetRangeMinto throw #3532 totestJointsCHANGELOG.md (choose one)
This change is