Use offsetalator in cudf::strings::reverse #15001
Conversation
davidwendt
added
2 - In Progress
| result->view().child(strings_column_view::offsets_column_index).data<size_type>(); | ||
| auto d_chars = result->mutable_view().head<char>(); | ||
| auto result = std::make_unique<column>(input.parent(), stream, mr); | ||
| auto sv = strings_column_view(result->view()); |
There was a problem hiding this comment.
Could result be const here?
There was a problem hiding this comment.
we do modify the column, const would be a bit misleading IMO 🤷
There was a problem hiding this comment.
The result should be elided on return which is kind of like a move which changes the object which would be prevented on a const object. I'd rather communicate to the compiler (and the reviewers) that this object is moved and not copied on return. I fear the const declaration would send the wrong hint to the compiler.
There was a problem hiding this comment.
FWIW, constness does not interfere with the copy elision, e.g. https://godbolt.org/z/TzPjshqrd
We'd need a std::move in the return if the copy elision didn't kick in, since result is not copyable.
I agree that const sends the wrong message regardless.
davidwendt
added
3 - Ready for Review
|
/merge |
Description
Updates
cudf::strings::reverseto use the offsetalator instead of hardcoded int32 type for offsets column data.Checklist