- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 216
 
Fix SOE from ResourceKey#compareTo #4238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix SOE from ResourceKey#compareTo #4238
Conversation
| 
           We should redirect it to the vanilla one so we don't accidentally return different results from different APIs.  | 
    
| 
           What is someone implements   | 
    
| 
           Moreover, vanilla comparison doesn't look reachable from API, currently it will always use adventure comparator  | 
    
          
 Thinking about this, I think we want to defer vanilla calls to adventure to be consistent. 
 The   | 
    
          
 With   | 
    
| 
           That should be fine, yes.  | 
    
| public int compareTo(final ResourceLocation location) { | ||
| return ((Key) this).compareTo((Key) (Object) location); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public int compareTo(final ResourceLocation location) { | |
| return ((Key) this).compareTo((Key) (Object) location); | |
| public int compareTo(final Object obj) { | |
| return ((Key) this).compareTo((Key) obj); | 
After looking at the implementations of ResourceLocation#compareTo(ResourceLocation) and Key#compareTo(Key), they seem completely equivalent to me. I don't think we need to overwrite one or the other. What I'm more worried about is the synthetic method ResourceLocation#compareTo(Object) which is casting and delegating to ResourceLocation#compareTo(ResourceLocation). I think this one should be overwritten to delegate to Key#compareTo(Key) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Didn't know it works via synthetic method. I guess now ResourceLocation#compareTo(ResourceLocation) should not be reachable from the API and no CCE should be thrown if anyone implements Key on their own (or if KeyMixin is removed which would result in Adventure KeyImpl being used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
No description provided.