I think we may have a use case for method handles here? #11
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces method handles to replace reflection-based method invocation throughout the math operators codebase. The main purpose is to modernize the reflection usage by leveraging Java's MethodHandle API for better performance and cleaner code.
Key changes:
- Replace
MethodwithMethodHandleacross all operator classes and interfaces - Update reflection utility methods to return
MethodHandleinstances instead ofMethodobjects - Modify exception handling to expect
WrongMethodTypeExceptioninstead ofNoSuchMethodException
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| BasicFunctionTest.java | Updated test to expect WrongMethodTypeException instead of NoSuchMethodException |
| OperatorInterface.java | Changed return type from Method to MethodHandle and simplified boolean logic |
| BasicFunction.java | Replaced Method with MethodHandle and updated invocation to use invokeExact |
| BasicComparisonOperator.java | Updated field types and constructor to use MethodHandle |
| BasicAlgebraicUnaryOperator.java | Replaced Method with MethodHandle and simplified error handling |
| BasicAlgebraicIntOperator.java | Updated to use MethodHandle throughout |
| BasicAlgebraicBinaryOperator.java | Replaced Method with MethodHandle and streamlined exception handling |
| AlgebraicIntOperator.java | Updated static method signature to accept MethodHandle |
| AlgebraicBinaryOperator.java | Commented out method resolution logic |
| AbstractAlgebraicIntOperator.java | Changed constructor and field to use MethodHandle |
| ReflectionUtils.java | Added utility methods for creating MethodHandle instances |
| PAdicIntegers.java | Updated static operator definitions to use new method handle utilities |
| try { | ||
| try { | ||
| return (R) method.invoke(t); | ||
| return (R) method.invokeExact(t); |
There was a problem hiding this comment.
Using invokeExact requires exact type matching at compile time. Since the method signature uses generic types <T, R>, this will likely fail at runtime. Consider using invoke() instead of invokeExact() for more flexible type handling.
| return (R) method.invokeExact(t); | |
| return (R) method.invoke(t); |
| default <E extends AlgebraicElement<E>> Method getMethodFor(E e) { | ||
| return e.getClass().getMethod(getMethod().getName(), getMethod().getParameterTypes()); | ||
| throw new UnsupportedOperationException(); | ||
| //return e.getClass().getMethod(getMethod().getName(), getMethod().getParameterTypes()); |
There was a problem hiding this comment.
The commented-out implementation suggests this method was previously functional. Throwing UnsupportedOperationException in a default method may break existing code that relies on this functionality. Consider providing a proper implementation or documenting why this change was necessary.
| //return e.getClass().getMethod(getMethod().getName(), getMethod().getParameterTypes()); | |
| return e.getClass().getMethod(getMethod().getName(), getMethod().getParameterTypes()); |
| E result = (E) method.invoke(element1, element2); | ||
| if (result == null) { | ||
| throw new InvalidAlgebraicResult("" + method + "(" + element1 + ',' + element2 + ") resulted null"); | ||
| throw new InvalidAlgebraicResult(method + "(" + element1 + ',' + element2 + ") resulted null"); |
There was a problem hiding this comment.
String concatenation with MethodHandle may not produce the expected readable output. MethodHandle.toString() typically returns internal implementation details rather than a user-friendly method description. Consider storing the method name separately or using a different approach for error messages.
| throw new InvalidAlgebraicResult(method + "(" + element1 + ',' + element2 + ") resulted null"); | |
| throw new InvalidAlgebraicResult(symbol + "(" + element1 + ", " + element2 + ") resulted null"); |
| return OperatorInterface.super.getMethodFor(e); | ||
| } | ||
| }*/ | ||
| return null; |
There was a problem hiding this comment.
Returning null from getMethodFor() may cause NullPointerException in code that expects a valid Method object. This appears to be incomplete migration where the method signature still returns Method but the implementation has been changed to accommodate MethodHandle.
Test Results 39 files 39 suites 8s ⏱️ For more details on these failures and errors, see this check. Results for commit 2d7e47f. |
No description provided.