ARROW-455: [C++] Add dtor to BufferOutputStream that calls Close() #269

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@wesm
Member
wesm commented Jan 5, 2017

Since Close() can technically fail, it's better to call it yourself (and it's idempotent), but this will help avoid a common class of bugs in small-scale use cases.

An alternative here is that we could remove all Close() calls from all destructors and possibly add a DCHECK(!is_open_) to the base dtor to force the user to close handles. The downside of this is that it makes RAII more difficult, so I'd prefer to leave the close-in-dtor even though it can fail in unusual scenarios.

@wesm wesm Add dtor to BufferOutputStream that calls Close()
Change-Id: I337d413985f3f23ce9c96691baaf961ff97477a6
821ee22
@wesm wesm changed the title from ARROW-455: Add dtor to BufferOutputStream that calls Close() to ARROW-455: [C++] Add dtor to BufferOutputStream that calls Close() Jan 5, 2017
@xhochy
xhochy approved these changes Jan 5, 2017 View changes

+1, LGTM

@asfgit asfgit pushed a commit that closed this pull request Jan 5, 2017
@wesm wesm ARROW-455: [C++] Add dtor to BufferOutputStream that calls Close()
Since `Close()` can technically fail, it's better to call it yourself (and it's idempotent), but this will help avoid a common class of bugs in small-scale use cases.

An alternative here is that we could remove all `Close()` calls from all destructors and possibly add a `DCHECK(!is_open_)` to the base dtor to force the user to close handles. The downside of this is that it makes RAII more difficult, so I'd prefer to leave the close-in-dtor even though it can fail in unusual scenarios.

Author: Wes McKinney <wes.mckinney@twosigma.com>

Closes #269 from wesm/ARROW-455 and squashes the following commits:

821ee22 [Wes McKinney] Add dtor to BufferOutputStream that calls Close()
320f587
@asfgit asfgit closed this in 320f587 Jan 5, 2017
@wesm wesm deleted the wesm:ARROW-455 branch Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment