DM-19674: Channel: check error status from astRead #51
Conversation
If the error status isn't checked, it can deceive us into thinking it's coming from somewhere else.
| @@ -49,6 +49,7 @@ Channel::~Channel() { | |||
|
|
|||
| std::shared_ptr<Object> Channel::read() { | |||
| AstObject *rawRet = reinterpret_cast<AstObject *>(astRead(getRawPtr())); | |||
| assertOK(rawRet); | |||
There was a problem hiding this comment.
I don't think it takes an argument.
There was a problem hiding this comment.
It takes two optional arguments: https://github.com/lsst/astshim/blob/master/include/astshim/base.h#L108
There was a problem hiding this comment.
Signature says it takes up to two objects that should be deallocated if the error check fails. We should really be capturing pointers like this in a smart pointer immediately and relying on destructors to take care of them, but that definitely seems out of scope for this ticket, and it's possible there was a valid reason for not doing so here.
There was a problem hiding this comment.
Weird, the others in this file don't seem to take any, but ok 😄
There was a problem hiding this comment.
Looks good. As discussed out-of-band, let's get this on master ASAP but make sure we do follow up eventually with a test case. I wouldn't be opposed to looking for other exception throws in astshim to see if they should get the same treatment.
I'm really getting out of scope now, but if there's global error state, asserting that it's clear before doing anything would also be worth considering.
If the error status isn't checked, it can deceive us into thinking it's
coming from somewhere else.