tobim commented on PR #48128: URL: https://github.com/apache/arrow/pull/48128#issuecomment-3536246658
While that is a smaller change it would probably work, I think the proposed change is superior. The supposed optimization of returning a const ref in the old implementation does not work anyways since the function bumps the ref-count internally in the way it checks if the array is already materialized. So to compare the ref-count changes in case the caller takes ownership: original implementation: +1 -1 +1 proposed implementation: +1 If the caller does not take ownership: original implementation: +1 -1 proposed implementation: +1 -1 Another, probably more important argument: ``` auto fieldarray = std::move(parent).field(n) ``` is a potential heap-use-after-free that is fixed with the proposed implementation. Also, "simple" is somewhat subjective, and returning a `const shared_ptr&` from a `const` function that internally modifies a mutable cache is implies certain assumptions. To me, that's nowhere near "simple". -- 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]
