Skip to content

Commit

Permalink
Change cudf::scalar copy and move constructors to protected (#8857)
Browse files Browse the repository at this point in the history
An instance of the `cudf::scalar` base class can currently through it's public copy constructor using the following example:
```
cudf::scalar b = cudf::numeric_scalar<int32_t>(8);
```

This line builds the numeric scalar instance, passes it to the base scalar's copy-ctor, and then destroys it.
(Simpler example here to show that this is happening: https://godbolt.org/z/hGEGvsqjq )
This means that the `b` instance is actually a plain `cudf::scalar` with no derived type.

Downcasting with dynamic_cast can runtime-check the object but we could have this line fail at compile time by making the `cudf::scalar` copy-ctor protected.

We could make the ctor explicit but I think making it protected would more clearly communicate the intention of the class.

This PR makes the `cudf::scalar` copy and move constructors protected to prevent instantiating a `cudf::scalar` at compile time.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Karthikeyan (https://github.com/karthikeyann)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #8857
  • Loading branch information
davidwendt authored Jul 28, 2021
1 parent 0078bf6 commit fa544be
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 15 deletions.
28 changes: 14 additions & 14 deletions cpp/include/cudf/scalar/scalar.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,10 @@ namespace cudf {
*/
class scalar {
public:
virtual ~scalar() = default;
scalar(scalar&& other) = default;

virtual ~scalar() = default;
scalar& operator=(scalar const& other) = delete;
scalar& operator=(scalar&& other) = delete;

/**
* @brief Construct a new scalar object by deep copying another.
*
* @param other The scalar to copy.
* @param stream CUDA stream used for device memory operations.
* @param mr Device memory resource to use for device memory allocation.
*/
scalar(scalar const& other,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
* @brief Returns the scalar's logical value type.
*/
Expand Down Expand Up @@ -103,6 +90,19 @@ class scalar {

scalar() = delete;

scalar(scalar&& other) = default;

/**
* @brief Construct a new scalar object by deep copying another.
*
* @param other The scalar to copy.
* @param stream CUDA stream used for device memory operations.
* @param mr Device memory resource to use for device memory allocation.
*/
scalar(scalar const& other,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
* @brief Construct a new scalar object.
*
Expand Down
2 changes: 1 addition & 1 deletion cpp/tests/binaryop/binop-fixture.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ struct BinaryOperationTest : public cudf::test::BaseFixture {
cudf::test::UniformRandomGenerator<uint8_t> rand_gen(r_min, r_max);
uint8_t size = rand_gen.generate();
std::string str{"ஔⒶbc⁂∰ൠ \tنж水✉♪✿™"};
return cudf::scalar_type_t<T>(string_view(str.data(), size));
return cudf::scalar_type_t<T>(str.substr(0, size));
}
};

Expand Down

0 comments on commit fa544be

Please sign in to comment.