Skip to content
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

Use isAssignableFrom instead of relying on ClassCastException #18350

Merged
merged 2 commits into from May 16, 2016

Conversation

Projects
None yet
4 participants
@uschindler
Copy link
Contributor

commented May 14, 2016

Title says it all :-)

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2016

I found more of those, updating PR in a minute...

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2016

I added more changes around the static casting. It would be good if somebody cross-checks, because some of checks were really crazy (AnalyzerCaster was completely strange to me - I think the fix is right and much simpler)!

@uschindler uschindler force-pushed the uschindler:painless_isAssignableFrom branch May 14, 2016

@uschindler uschindler force-pushed the uschindler:painless_isAssignableFrom branch to 8195ef9 May 14, 2016

@clintongormley clintongormley changed the title painless: Use isAssignableFrom instead of relying on ClassCastException Use isAssignableFrom instead of relying on ClassCastException May 15, 2016

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

I agree catching the cast makes the logic hard to understand. personally i find asSubclass much easier to reason about than isAssignableFrom (i think the verb from in the method throws me for a loop every time), but given that we want to return our own exceptions anyway and that there is some "or" logic in here, I think this is better here. Thanks for cleaning it up! cc: @jdconrad

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2016

personally i find asSubclass much easier to reason about than isAssignableFrom (i think the verb from in the method throws me for a loop every time)

I know those arguments - especially the "From" confuses. If you once understood how this method should be used, it is quite easy to understand:

If you have left.isAssignableFrom(right) then this only returns true if you could do the following assignment leftInstance = rightInstance - you see left stays left and right stays right. It returns false if you would need a cast and you would get a ClassCastException.

So don't think about super- or subclass, just think in terms of this assignment possible or not! :-)

The problem is similar to some people to understand like compareTo. It is the same: left.compareTo(right) is the same like Comparator.compare(left, right), so its just the order. It also brought me into the same mental loop until you realize this "order of arguments".

For the code in painless, the huge trycatches confuse more than the mental loop :-)

P.S.: If you look at Class.isSubclass()'s source, it calls isAssignableFrom internally, because it is the one that the JVM implements. It just wraps the ClassCastException afterwards. So In our current code, it does the same thing 2 times and each unwrapping exception. And its unreadable, too.

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2016

and that there is some "or" logic in here

Thats the real reason why I did it!

@jdconrad

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

Thanks @uschindler! LGTM. If I'm being completely honest, asSubclass was the one I found in examples, so I used that, but this is much cleaner.

@jdconrad jdconrad merged commit f664fa5 into elastic:master May 16, 2016

1 check passed

CLA Commit author has signed the CLA
Details

@uschindler uschindler deleted the uschindler:painless_isAssignableFrom branch May 16, 2016

@jdconrad jdconrad referenced this pull request May 17, 2016

Closed

Ongoing Painless Improvements #17992

15 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.