Looks great - the only minor quibble is that now StringConcat looks like
a regular javac context class; it even has an instance method - it's
therefore best to follow usual initialization pattern for javac components:
i.e.
protected static final Context.Key<StringConcat> concatKey = new
Context.Key<>();
public static StringCOncat instance(Context context) {
StringConcat instance = context.get(unlambdaKey);
if (instance == null) {
instance = new StringConcat(context);
}
return instance;
}
private StringConcat(Context context) {
context.put(concatKey, this);
//init all context dependent fields
}
So, this will make StringConcat a regular javac component that can be
instantiated with:
StringConcat.instance(context)
Which would be done inside Gen.
Note that this will mess up with the fact that your helpers extends
directly from StringConcat, so you will probably need to separate the
internal helper classes from the StringConcat API which will be used
form outside. Let me know if you need help in getting this wired up
correctly.
Regarding the tests - I note that you have a good combo test in the JDK
repo; one thing that this type of test doesn't cover is if you
accidentally get the right result with a bad bytecode. I'll leave the
decision as to whether to cover this to you (since the coverage provided
by the JDK test is adequate anyway).
MAurizio
On 27/11/15 17:34, Aleksey Shipilev wrote:
Hi Maurizio,
Updated webrevs:
http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.03/
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.04/
On 11/27/2015 04:03 AM, Maurizio Cimadamore wrote:
Thanks! The patch looks better, and having the code all in one place
definitively helps. I think there is still something that you can do to
improve the code - i.e. all strategies seem to do:
...
Then, all you have to do is to create the right helper given the command
line options, and that object will have an API that can be used by Gen
to perform string concact effectively, w/o switching and minimizing
duplication.
What do you think?
I agree, these does look better, see the updated webrev.
Thanks,
-Aleksey
