DM-12615: Add copy-constructors to astshim objects #38
Conversation
| */ | ||
| AstObject * getRawPtrCopy() const { | ||
| AstObject * rawPtrCopy = reinterpret_cast<AstObject *>(astCopy(getRawPtr())); | ||
| assertOK(rawPtrCopy); |
There was a problem hiding this comment.
I am not familiar with much of ast, will this nicely handle exceptions etc, if this assertion turns out to not be ok?
There was a problem hiding this comment.
Yes. If there is an error assertOK will annul rawPtrCopy and then raise an exception. This prevents memory leaks.
| @@ -105,7 +105,7 @@ class FrameDict : public FrameSet { | |||
|
|
|||
| @throws std::invalid_argument if two Frames in the FrameSet have the same non-empty domain. | |||
| */ | |||
| explicit FrameDict(FrameSet const &frameSet) : FrameSet(std::move(*frameSet.copy())), _domainIndexDict() { | |||
| explicit FrameDict(FrameSet const &frameSet) : FrameSet(frameSet), _domainIndexDict() { | |||
There was a problem hiding this comment.
It seems weird that this commit in history happens before the commit to add the copy constructor. In the end it wont matter much, just the timeline for things seems off to anyone reading git log
There was a problem hiding this comment.
I added it after adding C++ copy constructor and before adding Python support for copy constructor. So it's reasonable, but I agree it would be clearer to swap the last two
There was a problem hiding this comment.
I could combine the C++ and python copy constructor commits; I was basically waiting to see if there would be complaints about having the C++ copy constructors exposed to Python (but I prefer that if only to unit-test it!)
There was a problem hiding this comment.
I am fine with it being exposed to python. Swap if you can, otherwise it is ok I guess
python/astshim/test.py
Outdated
| chan.write(obj) | ||
| ss.sinkToSource() | ||
| if channelType == FitsChan: |
There was a problem hiding this comment.
This should be:
if channelType is FitsChan:
python/astshim/test.py
Outdated
| and all properties are identical | ||
| (including whether set or defaulted). | ||
| """ | ||
| self.assertEqual(obj1.className, obj2.className) |
There was a problem hiding this comment.
Why are you checking types like this and not
self.assertIs(type(obj1), type(obj2))
it seems more idiomatic to use actual type checking
There was a problem hiding this comment.
Things can get a bit weird for astshim classes with no direct AST equivalent: FrameDict is a wrapper around FrameSet and SeriesMap and ParallelMap are both basically CmpMaps.
However, I decided you were right and I made the test picker while adding support for the type being something other than expected in checkPersistence.
This also prompted me to improve pickle support for FrameDict so it round-trips correctly instead of coming back as a FrameSet.
| @@ -162,6 +162,13 @@ std::string Object::show(bool showComments) const { | |||
| return os.str(); | |||
| } | |||
|
|
|||
| Object::Object(AstObject *object) : _objPtr(object, &detail::annulAstObject) { | |||
There was a problem hiding this comment.
This seems to be the exact opposite of the commit message. If that was the intention, please change the message.
There was a problem hiding this comment.
Good catch. I fixed the message.
r-owen
force-pushed
the
tickets/DM-12615
branch
from
e1f6d2c
to
5696338
Compare
r-owen
force-pushed
the
tickets/DM-12615
branch
2 times, most recently
from
22bb869
to
c9ecffe
Compare
Make the FrameSet(frame, mapping, frame, options) constructor call FrameSet(frame, options) constructor, to reduce repetition.
Add these for all AST wrapper objects except channels (which cannot be copied) Include Python wrappers for the copy constructors and python unit tests.
This method was supposed to test persistence using three different channel classes, but in fact was only testing ast.Channel.
and use it to reduce some code repetition
Make all astshim persistable objects pickleable in Python by adding Object.__reduce__
Simplify test_object by using UnitMap's regular pickling for the multiprocessing test
Make ObjectTestCase.checkPersistence support testing for other types returned by a channel, as exptected for FrameDict. Make ObjectTestCase.assertObjectsIdentical pickier, while supporting the case just mentioned.
Use Channel instead of AST functions such as astChannel, in order to make the code more robust and minimze direct use of AST functions.
except the ones about defaulted functions which need a bigger fix
The documentation for CmpMap, SeriesMap and ParallelMap was incorrect regarding the effects of the latter two being a wrapper around the first.
r-owen
force-pushed
the
tickets/DM-12615
branch
from
c9ecffe
to
777fb5e
Compare
No description provided.