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

Add move constructors for string_scalar and struct_scalar #8428

Merged
merged 8 commits into from
Jun 8, 2021

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Jun 2, 2021

This PR adds some move constructors for string_scalar and struct_scalar that accept r-value references to existing buffers. By doing so, the input buffer is moved into the internal buffer of the newly constructed scalars instead of copying.

This also cleans up/rewrites doxygens for scalar.hpp.

Closes #8427.

@ttnghia ttnghia added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 2, 2021
@ttnghia ttnghia self-assigned this Jun 2, 2021
@ttnghia ttnghia requested a review from a team as a code owner June 2, 2021 15:36
@ttnghia ttnghia added libcudf blocker and removed improvement Improvement / enhancement to an existing function labels Jun 2, 2021
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all the comments reformatted?

cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 2, 2021

Why are all the comments reformatted?

I just want to enforce aesthetic consistency for them: some comments ended with a period (.) while some others don't. Other (line adjustment) may be due to some explicit reformatting done by my IDE.

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@48a2663). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8428   +/-   ##
===============================================
  Coverage                ?   82.95%           
===============================================
  Files                   ?      109           
  Lines                   ?    18091           
  Branches                ?        0           
===============================================
  Hits                    ?    15008           
  Misses                  ?     3083           
  Partials                ?        0           

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 48a2663...1e201c3. Read the comment docs.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think secretly this PR is more about adding periods to the comments. :)

I think this is a good addition, just some doc suggestions.

cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented Jun 3, 2021

Other (line adjustment) may be due to some explicit reformatting done by my IDE.

Please disable this and just use our mandated clang-format settings. Other than automatic line-wrapping to ensure passing clang-format tests, any reformatting of comments should be done manually and not on every file save or it will make PRs harder to review (and cause more conflicts, e.g. with my in-progress scalar.hpp refactor PR)

I had a hard time getting to the real changes in this PR.

@harrism harrism changed the title Add constructors accepting orphaned buffer for string_scalar and struct_scalar Add move constructors for string_scalar and struct_scalar Jun 3, 2021
@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 3, 2021

Please disable this and just use our mandated clang-format settings. Other than automatic line-wrapping to ensure passing clang-format tests, any reformatting of comments should be done manually and not on every file save or it will make PRs harder to review (and cause more conflicts, e.g. with my in-progress scalar.hpp refactor PR)

I had a hard time getting to the real changes in this PR.

I'm sorry about that. Typically it is disabled, I just accidentally called that this time.

@ttnghia ttnghia requested a review from harrism June 3, 2021 02:50
@ttnghia ttnghia requested a review from davidwendt June 3, 2021 02:50
@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 3, 2021

Rerun tests.

2 similar comments
@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 3, 2021

Rerun tests.

@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 4, 2021

Rerun tests.

Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(Sorry, I thought I had already approved this PR last week.)

@ttnghia
Copy link
Contributor Author

ttnghia commented Jun 8, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 90e29d9 into rapidsai:branch-21.08 Jun 8, 2021
@ttnghia ttnghia deleted the add_constructors_scalars branch June 9, 2021 12:10
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 feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Construct scalar by moving a free buffer
6 participants