Painless: Clean up FunctionRef #32644
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
I think the original name FunctionRef actually fit better into existing names within the codebase, eg we have BytesRef. The rename is also very difficult to review, as it is unclear what little changes there are within the file since it looks all new. Can you change the name back, and if the rename is that important, do it in a followup PR by itself?
| Class<?> interfaceType = painlessLookup.canonicalTypeNameToType(interfaceClass); | ||
| PainlessMethod interfaceMethod = painlessLookup.lookupPainlessClass(interfaceType).functionalMethod; | ||
| if (interfaceMethod == null) { | ||
| Class<?> interfaceType = painlessLookup.canonicalTypeNameToType(interfaceClass); |
There was a problem hiding this comment.
nit: seems like this is indented too much now?
There was a problem hiding this comment.
Fixed. For some reason the formatting in this file has a lot of issues.
| import static org.objectweb.asm.Opcodes.H_INVOKEVIRTUAL; | ||
| import static org.objectweb.asm.Opcodes.H_NEWINVOKESPECIAL; | ||
|
|
||
| public class FunctionReference { |
There was a problem hiding this comment.
It looks like the top level javadoc was lost in the rename?
There was a problem hiding this comment.
Added a new one. The old one wasn't descriptive.
|
@rjernst Thanks for the review! Ready for the next round. Renamed this back to FunctionRef; however, while we do have classes like BytesRef outside of Painless, we've been heading toward longer more descriptive file names in Painless, and I would like to change this back in a different PR. The file names in Painless are all over the place right now and I would like a stronger consistency moving forward as things are worked on. |
|
@rjernst Thanks again for the review! Will commit as soon as CI passes. |
This change consolidates all the logic for generating a FunctionReference (renamed from FunctionRef) from several arbitrary constructors to a single static function that is used at both compile-time and run-time. This increases long-term maintainability as it is much easier to follow when and how a function reference is being generated. It moves most of the duplicated logic out of the ECapturingFuncRef, EFuncRef and ELambda nodes and Def as well.
This change consolidates all the logic for generating a FunctionReference (renamed from FunctionRef) from several arbitrary constructors to a single static function that is used at both compile-time and run-time. This increases long-term maintainability as it is much easier to follow when and how a function reference is being generated. It moves most of the duplicated logic out of the ECapturingFuncRef, EFuncRef and ELambda nodes and Def as well.