-
Notifications
You must be signed in to change notification settings - Fork 920
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.
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
[REVIEW] [CUIO] Replace owning raw pointers with std::unique_ptr #5720
Conversation
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
Codecov Report
@@ Coverage Diff @@
## branch-0.15 #5720 +/- ##
==============================================
Coverage ? 86.38%
==============================================
Files ? 76
Lines ? 13041
Branches ? 0
==============================================
Hits ? 11265
Misses ? 1776
Partials ? 0 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just a question about the exceptional case.
cpp/src/io/comp/uncomp.cpp
Outdated
} | ||
return decompressor; | ||
return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you throw
/ CUDF_FAIL
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This keeps the original behavior - return null if the compression type is not supported.
I'll check if null is actually signalling an error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, returning null here causes an error later on, when trying to parse the data that was never successfully read. Added CUDF_FAIL
, removed code that assumes that the decompressor can be null.
…emove-cuio-new-calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better
…into remove-cuio-new-calls
HostDecompressor::Create
used to return a raw pointer to a newly created object. Because of this, the caller had a destructor.With this PR, a
unique_ptr
is returned instead, and the caller's destructor is removed.