alamb commented on code in PR #8417:
URL: https://github.com/apache/arrow-rs/pull/8417#discussion_r2376768923
##########
arrow-schema/src/field.rs:
##########
@@ -132,6 +132,12 @@ impl Hash for Field {
}
}
+impl AsRef<Field> for Field {
Review Comment:
this change means that the rust compiler with automatically make references
from owned Fields which seems strange to me -- among other things it makes it
less clear at the callsite that a function doesn't need an owned field
So like
```rust
let field = ...
my_function(field)
```
I would expect that my_function requires ownership of the field. If I saw
```rust
let field = ...
my_function(&field)
```
I would know the function doesn't need ownership
##########
arrow-schema/src/field.rs:
##########
@@ -1118,6 +1124,19 @@ mod test {
assert!(f1.cmp(&f3).is_lt());
}
+ #[test]
+ #[expect(clippy::needless_borrows_for_generic_args)] // intentional to
exercise various references
+ fn test_field_as_ref() {
+ fn accept_ref(_: impl AsRef<Field>) {}
Review Comment:
I think a more standard signature would be this:
```suggestion
fn accept_ref(_: &Field) {}
```
And then the compiler would call the various `AsRef` impls as appropriate
However, @tustvold said he thought it was fine on
https://github.com/apache/arrow-rs/issues/5819 and I would defer to his rust
expertise
--
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]
