westonpace commented on code in PR #13613:
URL: https://github.com/apache/arrow/pull/13613#discussion_r933392902
##########
cpp/src/arrow/engine/substrait/ext_test.cc:
##########
@@ -158,10 +152,10 @@ TEST_P(ExtensionIdRegistryTest, ReregisterFunctions) {
auto provider = std::get<0>(GetParam());
auto registry = provider->get();
- for (util::string_view name : kFunctionNames) {
- auto id = Id{kArrowExtTypesUri, name};
- ASSERT_RAISES(Invalid, registry->CanRegisterFunction(id,
name.to_string()));
- ASSERT_RAISES(Invalid, registry->RegisterFunction(id, name.to_string()));
+ for (Id function_id : kFunctionIds) {
Review Comment:
TBH, I don't really know. Small things we pass by value (e.g. `int`). A
`string_view` is a pair of pointers and generally considered small enough that
it is [better to pass by
value](https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/).
An `Id` is a pair of `string_view` objects. Does that mean it is small
enough? I'm not sure what the exact rules are here but the existing usage
seems to be treating it as a small value object and so I have tried to continue
to do so.
Perhaps @bkietz for a second opinion / further guidelines.
--
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]
