Painless: Move More Logic to PainlessLookup #32689
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
Looks good, but I think we should return null instead of using exceptions to indicate a class is not found.
| return handle; | ||
| try { | ||
| return painlessLookup.lookupRuntimeSetterMethodHandle(receiverClass, name); | ||
| } catch (IllegalArgumentException iae) { |
There was a problem hiding this comment.
I don't think we should use IAE as control flow. Can we return null if the the class is not found and base these special cases on that?
| return functionalInterfacePainlessMethod; | ||
| } | ||
|
|
||
| public PainlessMethod lookupRuntimePainlessMethod(Class<?> originalTargetClass, String methodName, int methodArity) { |
There was a problem hiding this comment.
Can you add at least a single line javadoc to these methods (and preferably the other ones as well here, although that could be in a followup)? It is getting difficult to understand what the differences in expected behavior are, eg based on a single word difference here of adding "runtime" to the method name, but taking the same args as lookupPainlessMethod above.
There was a problem hiding this comment.
I'd like to add documentation for all of these in a follow up PR as there are a few more changes lined up and I would prefer not to write it twice over the next couple weeks if that's okay.
|
@rjernst Thanks for the review. I changed all the methods in PainlessLookup to return null and react at the site of the calls instead of using exception handling as control flow. Ready for another round. |
|
@rjernst Thanks for the reviews! |
This moves some run-time lookups for methods and fields to the PainlessLookup.
4 participants
This moves some run-time lookups for methods and fields to the PainlessLookup.