ARROW-1500: [C++] Do not ignore return value from truncate in MemoryMa…#1116
ARROW-1500: [C++] Do not ignore return value from truncate in MemoryMa…#1116amirma wants to merge 1 commit intoapache:masterfrom
Conversation
cpp/src/arrow/io/file.cc
Outdated
There was a problem hiding this comment.
Can you use the CheckOpenResult function for this? Adding the errno to the error string generated there would be useful
There was a problem hiding this comment.
Ack. Perhaps I should rename "CheckOpenResult" to something more generic like "CheckFileOpsResult"?
|
The PR title is missing an "A" in |
amirma
changed the title
wesm
left a comment
wesm
left a comment
There was a problem hiding this comment.
+1 -- if you can fix the small nit I will merge once the build runs. Thank you for the contribution!
cpp/src/arrow/io/file.cc
Outdated
There was a problem hiding this comment.
Done. Thanks for reviewing!
amirma
left a comment
amirma
left a comment
There was a problem hiding this comment.
Done. Thanks for reviewing!
cpp/src/arrow/io/file.cc
Outdated
There was a problem hiding this comment.
Done. Thanks for reviewing!
|
Thanks, the Travis CI tubes are a bit clogged today so I may not be able to merge until later tonight or tomorrow morning |
|
@wesm Bah, I just noticed my patch has a bug; if truncate fails we will leak the file handle. I just resubmitted a fixed version. Thanks. |
amirma
force-pushed
the
arrow-1500
branch
2 times, most recently
from
cf6e78e to
15c5231
Compare
cpp/src/arrow/io/file.cc
Outdated
There was a problem hiding this comment.
I think this would be handled by RAII without this https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/file.cc#L473. If there is an esoteric failure to close the file handle then this would bubble up the error, but I'm not sure why that would occur
There was a problem hiding this comment.
Right, I missed that line in the dtor. Then I'll revert to the previous version of the patch.
|
Can you rebase? Not sure why there's a merge conflict now |
|
Rebased. |
|
This is failing with cpplint warnings you can use |
…appedFile::Create Author: Amir Malekpour <[email protected]>
|
Fixed lint errors. |
2 participants
…ppedFile::Create
Author: Amir Malekpour [email protected]