On Sun, 29 Oct 2023 07:51:48 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> @dholmes-ora Yes it helps avoid copying, especially if the copy constructor >> is non-trivial. And I think it is more idiomatic in C++ to use references >> here. > > Using a reference here leads to unnecessary overhead when `E` is small and > trivially copyable, unless the predicate function is inlined. Pass by value is > better in that case. Of course, as noted above, if `E` is "expensive" to copy > or non-copyable then a reference is needed. Boost has this thing called > `call_traits<E>::param_type` for this issue, but I don't recommend we copy > that. > > Idiomatic C++ makes the entire function a template parameter, as was suggested > earlier in this PR. That dodges this question entirely, leaving the parameter > passing decision to the predicate function where it belongs, rather than > having it imposed by GrowableArray::find. The find function just imposes the > requirement that the predicate satisfies the appropriate constraints, e.g. it > is callable on an element reference and returns convertible to bool. I agree we should be using a template-typed function instead of a function pointer here. I think a lot of our uses of function pointers in our code base would work better as template-typed args. See for example the `grow` argument (of template type `GFN`) at this point: https://github.com/openjdk/jdk/blob/d051f22284e7ccc288c658588f73da672d9bfacd/src/hotspot/share/utilities/unsigned5.hpp#L343C34-L343C34 In that case I cited, if the `grow` argument it were a function pointer instead of a template-typed function-like argument (Kim what’s the right term here? “functor”??), then performance and flexibility would be unacceptably harmed. I think the `find` argument is just the same kind of thingy. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1376940244
