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.

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

Minor I/O code quality improvements #17105

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions cpp/include/cudf/io/datasource.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class datasource {
template <typename Container>
static std::unique_ptr<buffer> create(Container&& data_owner)
{
return std::make_unique<owning_buffer<Container>>(std::move(data_owner));
return std::make_unique<owning_buffer<Container>>(std::forward<Container>(data_owner));
}
};

Expand Down Expand Up @@ -335,26 +335,33 @@ class datasource {
template <typename Container>
class owning_buffer : public buffer {
public:
// Require that the argument passed to the constructor be an rvalue (Container&& being an rvalue
// reference).
static_assert(std::is_rvalue_reference_v<Container&&>,
"The container argument passed to the constructor must be an rvalue.");

/**
* @brief Moves the input container into the newly created object.
*
* @param data_owner The container to construct the buffer from (ownership is transferred)
* @param moved_data_owner The container to construct the buffer from. Callers should explicitly
* pass std::move(data_owner) to this function to transfer the ownership.
*/
owning_buffer(Container&& data_owner)
: _data(std::move(data_owner)), _data_ptr(_data.data()), _size(_data.size())
owning_buffer(Container&& moved_data_owner)
: _data(std::move(moved_data_owner)), _data_ptr(_data.data()), _size(_data.size())
{
}

/**
* @brief Moves the input container into the newly created object, and exposes a subspan of the
* buffer.
*
* @param data_owner The container to construct the buffer from (ownership is transferred)
* @param moved_data_owner The container to construct the buffer from. Callers should explicitly
* pass std::move(data_owner) to this function to transfer the ownership.
* @param data_ptr Pointer to the start of the subspan
* @param size The size of the subspan
*/
owning_buffer(Container&& data_owner, uint8_t const* data_ptr, size_t size)
: _data(std::move(data_owner)), _data_ptr(data_ptr), _size(size)
owning_buffer(Container&& moved_data_owner, uint8_t const* data_ptr, size_t size)
: _data(std::move(moved_data_owner)), _data_ptr(data_ptr), _size(size)
{
}

Expand Down
6 changes: 5 additions & 1 deletion cpp/src/io/utilities/file_io_utilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,11 @@ class cufile_shim {

~cufile_shim()
{
if (driver_close != nullptr) driver_close();
// Explicit cuFile driver close should not be performed here to avoid segfault. However, in the
// absence of driver_close(), cuFile will implicitly do that, which in most cases causes
// segfault anyway. TODO: Revisit this conundrum once cuFile is fixed.
// https://github.com/rapidsai/cudf/issues/17121
vuule marked this conversation as resolved.
Show resolved Hide resolved

if (cf_lib != nullptr) dlclose(cf_lib);
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/io/utilities/file_io_utilities.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class cufile_shim;
/**
* @brief Class that provides RAII for cuFile file registration.
*/
struct cufile_registered_file {
class cufile_registered_file {
void register_handle();

public:
Expand Down
Loading