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

Change cudf::scalar copy and move constructors to protected #8857

Merged
merged 5 commits into from
Jul 28, 2021

Conversation

davidwendt
Copy link
Contributor

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.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 26, 2021
@davidwendt davidwendt self-assigned this Jul 26, 2021
@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #8857 (89a59b3) into branch-21.10 (18f7c01) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 89a59b3 differs from pull request most recent head ee30d95. Consider uploading reports for the commit ee30d95 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.10    #8857      +/-   ##
================================================
- Coverage         10.67%   10.59%   -0.09%     
================================================
  Files               110      116       +6     
  Lines             18271    19034     +763     
================================================
+ Hits               1951     2017      +66     
- Misses            16320    17017     +697     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/column.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/lists.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/numerical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/struct.py 0.00% <ø> (ø)
python/cudf/cudf/core/dataframe.py 0.00% <ø> (ø)
python/cudf/cudf/core/frame.py 0.00% <ø> (ø)
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 904222b...ee30d95. Read the comment docs.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jul 27, 2021
@davidwendt davidwendt marked this pull request as ready for review July 27, 2021 14:15
@davidwendt davidwendt requested a review from a team as a code owner July 27, 2021 14:15
@davidwendt davidwendt requested review from vyasr, ttnghia and karthikeyann and removed request for ttnghia July 27, 2021 14:15
@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit fa544be into rapidsai:branch-21.10 Jul 28, 2021
@davidwendt davidwendt deleted the protect-scalar-ctors branch July 28, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants