[GitHub] [lucene-solr] dweiss commented on a change in pull request #1837: LUCENE-7882: First idea of using Java 15 hidden anonymous classes for Lucene expressions

[GitBox <gi...@apache.org>]

dweiss commented on a change in pull request #1837:
URL: https://github.com/apache/lucene-solr/pull/1837#discussion_r484194383



##########
File path: lucene/expressions/src/test/org/apache/lucene/expressions/js/TestCustomFunctions.java
##########
@@ -263,7 +263,7 @@ public void testThrowingException() throws Exception {
     PrintWriter pw = new PrintWriter(sw);

Review comment:
       Perhaps we should import assertj for tests. These assertions are so much cleaner with assertj. Don't know whether hamcrest equivalent exist (maybe it does).

##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -206,6 +247,12 @@ private Expression compileExpression(ClassLoader parent) throws ParseException {
       throw new IllegalStateException("An internal error occurred attempting to compile the expression (" + sourceText + ").", exception);
     }
   }
+  

Review comment:
       I wouldn't do this, to be honest. Rethrow Error and RuntimeException subclasses as they were, wrap anything else in a runtime exception with an appropriate caused-by delegate. Makes it easier to reason about the code should something happen.

##########
File path: lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
##########
@@ -191,9 +220,21 @@ private Expression compileExpression(ClassLoader parent) throws ParseException {
 
     try {
       generateClass(getAntlrParseTree(), classWriter, externalsMap);
-
-      final Class<? extends Expression> evaluatorClass = new Loader(parent)
-        .define(COMPILED_EXPRESSION_CLASS, classWriter.toByteArray());
+      
+      final Class<? extends Expression> evaluatorClass;

Review comment:
       Just a suggestion. My personal preference is to set up a supplier (or a function) statically, depending on conditions, then just use that later forever. In this case you could have something like: 
   ````
   static Function<byte[], Class<? extends Expression>> classLoader;
   static {
     if (MH_...) {
       classLoader = (bytes) -> {};
     } else {
       classLoader = (bytes) -> {};
     }
   }
   ````
   which would be defined once and used without the knowledge of its internal implementation details later on, forever. Just a thought.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org