opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
759 stars 308 forks source link

Add back CopyConstructors for instantiated array templates #3709

Closed aymanhab closed 4 months ago

aymanhab commented 4 months ago

Fixes issue #0

Brief summary of changes

Updating the Array class resulted in missing Copy constructors for all Array<> classes in Java/Matlab. This PR adds them back so that GUI code and possible user code that depends on them continue to work.

Testing I've completed

Building, checking resulting Java files

Looking for feedback on...

  1. Should we try to do this for Python as well? Not sure how this is handled now.
  2. If 1 is affirmative, should we make this change in Array directly or keep it off as a SWIG limitation

CHANGELOG.md (choose one)


This change is Reviewable

aymanhab commented 4 months ago

@adamkewley for your feedback, thank you

adamkewley commented 4 months ago

If this is because SWIG doesn't emit them in the absence of a copy constructor in C++, it might be easier to write Array(const Array&) = default; in Array.h to tell the compiler that it has to emit them?

Maybe rule of 5 it:

To ensure that all possible ctor/assignments are covered

(note: the C++ compiler will never emit template methods in the binary, but i'm guessing this relates to SWIG's parser)

aymanhab commented 4 months ago

Thanks @adamkewley, that also works and was my other question above. Since you seem to agree with that I'll undo the swig change and implement in Array.h. I also added a Java test case to exercise so if the test pass then we're good. Thank you

aymanhab commented 4 months ago

Done, and tests pass so ready to go.

adamkewley commented 4 months ago

Great! Sorry for the hiccup, I'll try to keep the behavior in mind in the future!

aymanhab commented 4 months ago

Thanks much @adamkewley much appreciated, and no worries at all 💯