Code review comment for lp:~zorba-coders/zorba/fn_envvars

Chris Hillery (ceejatec) wrote :

Regarding point 2: Remember that the StreamReleaser and the istream must always be kept together as a pair, and the StreamReleaser must be the only way that you clean up the istream. Here, you are deleting the istream directly (bad), and by setting the StreamReleaser on theStreamResource to nullptr, you're breaking up the pair (also bad).

In this case, the StreamResource is already maintaining the pair, and so you can actually make your life easier by just maintaining ownership of the StreamResource and making sure that IT is deleted when you're done. In that case you never have to look at StreamReleaser at all. To do this:

 - In FnUnparsedTextLinesIterator::nextImpl(), change lResource.get() to lResource.release(). By doing this, you take ownership of the StreamResource (so that the auto_ptr<> won't delete it).

 - Later in the same function, don't call state->theStreamResource->setStreamReleaser(nullptr);.

 - In FnUnparsedTextLinesIteratorState::reset(), just delete theStreamResource, and then set theStream and theStreamResource to 0. Deleting the StreamResource will correctly clean up the stream by calling the StreamReleaser.

If I understand Matthias, you will also need to delete theStreamResource in the state's destructor, in case of exceptions. (That's why you set theStreamResource to 0 in reset(), so you can't accidentally delete it twice.)

« Back to merge proposal