Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DM-19674: Channel: check error status from astRead #51

Merged
merged 1 commit into from
May 10, 2019
Merged

DM-19674: Channel: check error status from astRead #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Channel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ Channel::~Channel() {

std::shared_ptr<Object> Channel::read() {
AstObject *rawRet = reinterpret_cast<AstObject *>(astRead(getRawPtr()));
assertOK(rawRet);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it takes an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes two optional arguments: https://github.com/lsst/astshim/blob/master/include/astshim/base.h#L108

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, the others in this file don't seem to take any, but ok 😄

if (!rawRet) {
throw std::runtime_error("Could not read an AST object from this channel");
}
Expand Down