Closed adamreeve closed 1 week ago
The equals method is renamed rather than overridden as the native equals method could have a different implementation than the C# one. In this way the user can choose which one to take.
I'm not sure if it makes sense to have both versions of to_string
available.
Can you name some concrete cases where this happens? (I'm not at a pc right now.)
So I noticed this also affects Uri.ToString
from GLib, and there is a warning logged in the CI builds about this, eg. see https://github.com/gircore/gir.core/actions/runs/9826647090.
But there are quite a few methods that cause this error in the Arrow code base. Some examples:
I don't really see a good reason to want to be able to use the base object.ToString
implementation, and think overriding makes sense to me, although one argument for not overriding ToString
and renaming to NativeToString
might be to allow users to provide a custom ToString
override.
I checked again and agree that we don't need a NativeToString
method.
If a user wants to have their own ToString
they can define a custom method and just call it.
One question remains. There is already a new modifier for functions.
So I wonder if we should prefer override
or new
? I would tend to new
as to still have the original function available if needed via a cast in general. I don't know if this is very important for ToString
though. But having a special case for ToString
in the first iteration of this fix is a bit much perhaps. So let's start with new
and add a special case for ToString
with override
if there is concrete need.
What do you think?
P.S.: if you agree and implement it please add a new helper method to Method which will probably be similar to the function equivalent.
I'm not sure about this, I would have gone with override
rather than new
, as otherwise there are situations where you need to cast to the concrete type to access the new method rather than the one from object
. And string interpolation with $"{myObj}"
for example calls object.ToString
rather than the new method, which might not be desired.
But that's my thoughts on ToString
specifically, I agree that new
probably makes sense as the default approach when a method hides another method. And yes if we want to change the behaviour for ToString
as a special case that can be done later. So I'm happy to just use the new
modifier for now.
And thanks for the pointer about the HidesFunction
method!
In the Arrow GObject library there are some
to_string()
methods on derivable types that result in warnings like:This is only a warning rather than an error, but we build with
TreatWarningsAsErrors
enabled, and it would be nice to fix this.I can see there's a
RecordEqualsMethodCollidesWithGeneratedCode
class that fixes a similar problem withEquals
methods in Records by renaming them toNativeEquals
, so a similar fix would be to rename these toNativeToString
. It might make sense to add anoverride
modifier instead though as it seems like it would generally make sense to use the nativeto_string
method to implementobject.ToString
.I'm happy to implement the fix for this after we agree on how to do it.