Make Object::_document RAII-safe

adamkewley commented 8 months ago

Fixes nit

Brief summary of changes

Changes Object::_document to a shared_ptr to clarify memory handling

Testing I've completed

None, I'm assuming that the original implementation doesn't rely on the raw pointer having an unusual lifetime

Looking for feedback on...

CHANGELOG.md (choose one)

updated.

This change is

adamkewley commented 8 months ago

LGTM 👍

Thanks for reading into this @tkuchida - I'll merge it after I do one last check on setDocument: one thing I'm realizing is that setDocument's old semantics are purely referential/leaky whereas the new one assumes control over whatever it receives, which might alter behavior

tkuchida commented 8 months ago

Sounds good, @adamkewley. (All tests are passing, so if a revision is required, I recommend beefing up :cow: the test cases to cover the difference.)

adamkewley commented 8 months ago

All uses of setDocument in the opensim-core tree are fine. E.g.:

OpenSim/Tools/InverseDynamicsTool.cpp:624: setDocument(new XMLDocument(newFileName));
OpenSim/Tools/InverseKinematicsTool.cpp:361: setDocument(new XMLDocument(newFileName));
OpenSim/Tools/InverseKinematicsTool.cpp:395: setDocument(new XMLDocument(newFileName));

Quick search through a spattering of open-source OpenSim API projects:

opensim-org/opensim-gui contains no references to it (it does contain references to a setDocument symbol, but it's for InstructionsTopComponent.java, no OpenSim::Model
ComputationalBiomechanicsLab/opensim-creator contains no references to it
tgeijten/scone-core contains no references to it
rcnl-org/nmsm-core contains no references to it
perfanalytics/pose2sim: no references to it

So I think it's safe to assume that adding unconditional ownership to the setDocument API (as in, the previous version would always leak any overwritten XMLDocument) is probably ok... probably.

opensim-org / opensim-core