Three defects in Chrono's copy constructors, grouped because they share a root cause: a hand-written
copy constructor that does not account for every member. All three were measured, not inferred, and
none of them reports an error. Found while investigating #507, where the first one explains why the
reporter's approach could never have worked.
1. ChLink's copy constructor discards both attached bodies
ChLink::ChLink(const ChLink& other) sets both body pointers to null rather than copying them:
ChLink::ChLink(const ChLink& other) : ChLinkBase(other) {
m_body1 = nullptr;
m_body2 = nullptr;
react_force = other.react_force;
react_torque = other.react_torque;
}
So any copy or Clone() of a link is silently detached from the bodies it constrained. Measured with a
ChLinkTSDA initialized against two bodies: GetBody1() is non-null before the copy and null after.
If this is intentional, because a copy cannot meaningfully share the original's bodies, then it would
help to say so in a comment, since the failure is otherwise invisible. It also means a container of
links held by value can never work, which is worth documenting.
2. Copying a ChLinkTSDA with a registered ODE rewinds its states
ChLinkTSDA's copy constructor calls RegisterODE(other.m_ode_fun), and RegisterODE runs
functor->SetInitialConditions(m_states, *this). Nothing assigns m_states = other.m_states, so the
copy receives the initial conditions rather than the current ones. A Clone() taken during a
simulation silently rewinds the link's internal ODE state to t = 0.
Measured with a two-state ODE whose initial conditions are [100, 200]:
original, freshly registered : states = [100, 200]
original, after stepping : states = [7.5, 8.25]
the COPY : states = [100, 200] <-- rewound
Clone() of the original : states = [100, 200] <-- rewound
SetInitialConditions() calls triggered by the copy: 1
3. ChObj's copy constructor never assigns m_tag
ChObj::ChObj(const ChObj& other) {
m_identifier = GenerateUniqueIdentifier();
m_name = other.m_name;
ChTime = other.ChTime;
} // m_tag is never assigned
ChObj() initializes m_tag(-1), but the copy constructor leaves it holding whatever occupied that
memory. Reading it is undefined behavior. Because this is ChObj, it affects essentially every object
in Chrono. Measured with no serialization involved:
| Case |
GetTag() |
| default-constructed link |
-1 |
| copy-constructed link |
447 |
| source tagged 4242, its copy |
0 |
Clone() of the tagged link |
447 |
ChBody tagged 77, its copy |
32765 |
There are two defensible fixes and the choice is a semantics decision for the maintainers rather than
a bug fix. My suggestion is the first, for consistency with m_name, which the copy constructor does
carry over:
m_tag = other.m_tag; so a copy inherits the source's tag.
m_tag = -1; on the view that a tag names one specific object, so duplicating it would create two
objects claiming the same identity.
Either is fine. What is not defensible is the current state, since no reading of C++ makes an
uninitialized read correct. Note that item 3 becomes visible in serialized output once #774 lands, for
the simple reason that before it the whole record was garbage anyway.
Three defects in Chrono's copy constructors, grouped because they share a root cause: a hand-written
copy constructor that does not account for every member. All three were measured, not inferred, and
none of them reports an error. Found while investigating #507, where the first one explains why the
reporter's approach could never have worked.
1.
ChLink's copy constructor discards both attached bodiesChLink::ChLink(const ChLink& other)sets both body pointers to null rather than copying them:So any copy or
Clone()of a link is silently detached from the bodies it constrained. Measured with aChLinkTSDAinitialized against two bodies:GetBody1()is non-null before the copy and null after.If this is intentional, because a copy cannot meaningfully share the original's bodies, then it would
help to say so in a comment, since the failure is otherwise invisible. It also means a container of
links held by value can never work, which is worth documenting.
2. Copying a
ChLinkTSDAwith a registered ODE rewinds its statesChLinkTSDA's copy constructor callsRegisterODE(other.m_ode_fun), andRegisterODErunsfunctor->SetInitialConditions(m_states, *this). Nothing assignsm_states = other.m_states, so thecopy receives the initial conditions rather than the current ones. A
Clone()taken during asimulation silently rewinds the link's internal ODE state to t = 0.
Measured with a two-state ODE whose initial conditions are
[100, 200]:3.
ChObj's copy constructor never assignsm_tagChObj()initializesm_tag(-1), but the copy constructor leaves it holding whatever occupied thatmemory. Reading it is undefined behavior. Because this is
ChObj, it affects essentially every objectin Chrono. Measured with no serialization involved:
GetTag()Clone()of the tagged linkChBodytagged 77, its copyThere are two defensible fixes and the choice is a semantics decision for the maintainers rather than
a bug fix. My suggestion is the first, for consistency with
m_name, which the copy constructor doescarry over:
m_tag = other.m_tag;so a copy inherits the source's tag.m_tag = -1;on the view that a tag names one specific object, so duplicating it would create twoobjects claiming the same identity.
Either is fine. What is not defensible is the current state, since no reading of C++ makes an
uninitialized read correct. Note that item 3 becomes visible in serialized output once #774 lands, for
the simple reason that before it the whole record was garbage anyway.