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

[C++] Pass shared_ptr<DataType> by value to parametric type constructors #37891

Open
felipecrv opened this issue Sep 26, 2023 · 3 comments
Open

Comments

@felipecrv
Copy link
Contributor

Describe the enhancement requested

Types like ListType and factory functions like list take a const std::shared_ptr<DataType>& instead of a std::shared_ptr<DataType> that could be moved into the newly constructed ListType without the need of bumping a refcount and still support cases where caller can only give it a reference.

Currently:

std::shared_ptr<DataType> list(const std::shared_ptr<DataType>& value_type) {
  return std::make_shared<ListType>(value_type);
}

After this issue is fixed:

std::shared_ptr<DataType> list(std::shared_ptr<DataType> value_type) {
  return std::make_shared<ListType>(std::move(value_type));
}

When working on this, we should take the time to go through every callsite and decide if a std::move() is possible without breaking correctness — things could break if the parameter is used after it's moved into the new instance of a parametric type.

Component(s)

C++

@Andreas-Hadjiantoni
Copy link

@felipecrv should we close this issue? I see that it was resolved by this PR

@felipecrv
Copy link
Contributor Author

felipecrv commented Jun 30, 2024

@felipecrv should we close this issue? I see that it was resolved by this PR

Not really because there are many other functions and constructors that should go through to this. list and ListViewType were just examples.

The steps for working on this:

  1. Identify a function that could go from taking shared_ptr& to shared_ptr (usually constructors)
  2. find the callers and do the same transformation on them
  3. add std::move at all callsites but make sure (by code review) that the value that is std::move'd from is not used after the call

@chrisxu333
Copy link

@felipecrv If nobody is working on this issue and it's still needed, can I take over?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants