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

[felipecrv]

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]

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

[felipecrv]

@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]

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

[felipecrv]

Of course.

[chrisxu333]

#45466 I've make an initial commit that does a similar change to the SliceBuffer function. @felipecrv Could you help check if I understand the intent of this issue correctly? If so I'll go ahead and make other changes accordingly.

[Shivani622]

@felipecrv Could you please assign this issue to me if it is still open?

[chrisxu333]

@Shivani622 I'm still working on it.

[pitrou]

Issue resolved by pull request 45466 https://github.com/apache/arrow/pull/45466

[felipecrv]

I will re-open this issue because there many occurrences of const shared_ptr<T> & in constructors that should change to shared_ptr<T>. I don't expect a single PR to fix all of them.

[singh1203]

@felipecrv I would love to work on it as I am new to C++.

[mapleFU]

@singh1203 feel free to check and add some

[kapoisu]

@felipecrv Should this be applied to other movable classes (e.g. std::string) or to std::shared_ptr only?

For other classes I think it'd be safe to only change it at the lowest level (constructors/factories). Sometimes internal references are kept as local variables, thus changing all of them requires extra care.

On the other hand, if some functions do not own the objects passed in, shouldn't we just use const/non-const reference to the type itself and dereference the std:shared_ptr when calling it rather than const reference to std::shared_ptr?

[pitrou]

Issue resolved by pull request 46202 https://github.com/apache/arrow/pull/46202