hsutter commented on PR #51270:
URL: https://github.com/apache/arrow/pull/51270#issuecomment-5985364095
I think this is now ready for review, please. (There's one check that fails
but it seems to be a timeout I'm not sure I can fix?)
As before, even separately from merging I'd be curious about any performance
test differences if we could please run those too -- my hope is to get rid of a
meaningful set of unneeded `shared_ptr` copies, and whether the performance
difference is detectable would be useful feedback.
### About the current state of the PR
This update is mostly as generated by my updated Claude skill, where the
difference from the first review is that if every definite last use is a copy
from the parameter it now correctly leaves the parameter as pass by value and
adds `std::move` to each definite last use. Before pushing the branch I took a
pass over Claude's proposed diffs and:
- manually verified each of the changes that were outside test files
(they all appear as intended to me);
- manually verified the changes in the first dozen or so test files (they
looked good); and
- skimmed most of the other test files;
and then I cleaned up CI failures:
- mostly lint/style such as formatting conventions; and
- one change I reverted because it didn't actually compile due to callee
const-ness that Claude didn't notice, and I didn't see until CI because it
looks like that file wasn't built when I ran `cmake --build build
--clean-first` in my local `arrow/cpp`.
Note: I suspect there are more cases of unneeded `shared_ptr` copies than
Claude found on this pass, but I want to see if this initial pass is useful to
you first before I look for more.
Thanks again for the feedback and encouragement so far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
