New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-11791: Fix memory leak in isSeries #28
Conversation
They had not been updated for the change in axis order for applyForward and applyInverse
and Mapping.then, Mapping.under and Frame.under to indicate that the returned mapping contains shallow copies of the input mappings.
Expand the testing of shallow and deep copying and deleting AST objects when done with them. Expand the test of FrameSet to more thoroughly test FrameSet.getMapping and FrameSet.remap.
It turns out FrameSet.remapFrame used a shallow copy of the frame; for better consistency with FrameSet.addFrame, etc., I changed this to make a deep copy, and updated the unit test accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks.
@@ -97,6 +97,10 @@ class CmpMap : public Mapping { | |||
bool getSeries() { return detail::isSeries(reinterpret_cast<AstCmpMap *>(getRawPtr())); } | |||
|
|||
protected: | |||
virtual std::shared_ptr<Object> copyPolymorphic() const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment string saying what this is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every single subclass of Object has one of these and it is documented there as something each subclass must override. I'm reluctant to add this to all subclasses on this ticket but will file a new one if you like. Doxygen already copies the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. Leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I managed to miss this is that Mapping defines it. I wonder if I should eliminate that and copy
, since Mapping is an abstract class.
No description provided.