-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[C++] Replace shared_ptr creation hack before AppendScalarImpl instantiation #33872
Comments
lidavidm
pushed a commit
that referenced
this issue
Jan 26, 2023
…33866) `AppendScalarImpl` was created for appending scalars based on a contiguous array of `std::shared_ptr<Scalar>`, so when a single `Scalar` reference is passed, a dummy `shared_ptr` is created before the instantiation of `AppendScalarImpl`. ```cpp std::shared_ptr<Scalar> shared{const_cast<Scalar*>(&scalar), [](Scalar*) {}}; return AppendScalarImpl{&shared, &shared + 1, n_repeats, this}.Convert(); ``` This seems non-idiomatic [1] and likely inefficient as a control block needs to be heap-allocated for the `shared_ptr`. An alternative way (that simplifies `AppendScalarImpl` as well) is a custom iterator class the wraps the source iterator with an extra dereference operation. [1] Pointers to `shared_ptr` as class members set a bad-style precedent ### Component(s) C++ * Closes: #33872 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
sjperkins
pushed a commit
to sjperkins/arrow
that referenced
this issue
Feb 10, 2023
…calar (apache#33866) `AppendScalarImpl` was created for appending scalars based on a contiguous array of `std::shared_ptr<Scalar>`, so when a single `Scalar` reference is passed, a dummy `shared_ptr` is created before the instantiation of `AppendScalarImpl`. ```cpp std::shared_ptr<Scalar> shared{const_cast<Scalar*>(&scalar), [](Scalar*) {}}; return AppendScalarImpl{&shared, &shared + 1, n_repeats, this}.Convert(); ``` This seems non-idiomatic [1] and likely inefficient as a control block needs to be heap-allocated for the `shared_ptr`. An alternative way (that simplifies `AppendScalarImpl` as well) is a custom iterator class the wraps the source iterator with an extra dereference operation. [1] Pointers to `shared_ptr` as class members set a bad-style precedent ### Component(s) C++ * Closes: apache#33872 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
gringasalpastor
pushed a commit
to gringasalpastor/arrow
that referenced
this issue
Feb 17, 2023
…calar (apache#33866) `AppendScalarImpl` was created for appending scalars based on a contiguous array of `std::shared_ptr<Scalar>`, so when a single `Scalar` reference is passed, a dummy `shared_ptr` is created before the instantiation of `AppendScalarImpl`. ```cpp std::shared_ptr<Scalar> shared{const_cast<Scalar*>(&scalar), [](Scalar*) {}}; return AppendScalarImpl{&shared, &shared + 1, n_repeats, this}.Convert(); ``` This seems non-idiomatic [1] and likely inefficient as a control block needs to be heap-allocated for the `shared_ptr`. An alternative way (that simplifies `AppendScalarImpl` as well) is a custom iterator class the wraps the source iterator with an extra dereference operation. [1] Pointers to `shared_ptr` as class members set a bad-style precedent ### Component(s) C++ * Closes: apache#33872 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the enhancement requested
AppendScalarImpl
was created for appending scalars based on a contiguous array ofstd::shared_ptr<Scalar>
, so when a singleScalar
reference is passed, a dummyshared_ptr
is created before the instantiation ofAppendScalarImpl
.This seems non-idiomatic [1] and likely inefficient as a control block needs to be heap-allocated for the
shared_ptr
. An alternative way (that simplifiesAppendScalarImpl
as well) is a custom iterator class the wraps the source iterator with an extra dereference operation.[1] Pointers to
shared_ptr
as class members set a bad-style precedentComponent(s)
C++
The text was updated successfully, but these errors were encountered: