apache / arrow

Apache Arrow is the universal columnar format and multi-language toolbox for fast data interchange and in-memory analytics
https://arrow.apache.org/
Apache License 2.0
14.53k stars 3.54k forks source link

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

Open felipecrv opened 1 year ago

felipecrv commented 1 year ago

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 commented 4 months ago

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

felipecrv commented 4 months ago

@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