search

CaffeineMC / hydrogen-fabric

Things of which are too dangerous to put in Lithium.
GNU Lesser General Public License v3.0
511 stars 66 forks source link

Intern Property instances so reference equality is guaranteed #22

Open magneticflux- opened 3 years ago

magneticflux- commented 3 years ago

Closes #19

2No2Name commented 3 years ago

I think the asymmetry of Object::equals is problematic. Is there any guarantee that WeakHashMap handles this the way you intend on all platforms? Could there be a platform where equals is called on the stored value leading to this breaking? Would it be possible to instead use the Property as the key and the value being the key wrapped in a WeakReference? Then getting the value for any equal Property will return the original object, and you still keep the WeakReference behavior of the WeakHashMap.

magneticflux- commented 3 years ago

I don't know of any implementations that don't call equals on the new object, but this change doesn't seem to have any downsides and it's always good to be cautious, especially with undefined behavior!

NebelNidas commented 3 years ago

Why hasn't this been merged yet? Is it still not safe to use?

malte0811 commented 3 years ago

This doesn't explain why it isn't merged yet (since I'm not the one who would need to merge it), but I'm not sure whether this is the right approach. Nothing is stopping modders from adding their own property implementations, which would not be interned by this PR and continue to cause issues.

Maybe I just don't know Yarn names, but I would have expected one of the mixins to target the property implementation for generic enums, rather than the Direction-specific one?

magneticflux- commented 3 years ago

@malte0811 That's true, if a mod implements Property<T> on its own, it won't be cached. That isn't a big issue though because that mod implementation typically won't be used by other mods.

You're right about the mixin, I need to make one that targets the EnumProperty explicitly. Static methods can't be overridden, so the DirectionProperty mixin is still needed.