Hi Aleksey,
> Yes, thanks for more polishing, I have uploaded the updates to:
> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.06/
Maybe one more:
1342 Class[] cls = new Class[cnt];
1343 for (int i = 0; i < cnt; i++) {
1344
Yes, thanks for more polishing, I have uploaded the updates to:
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.06/
http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.04/
More reviews, please!
Thanks,
-Aleksey
On 11/30/2015 12:34 PM, Andrej Golovnin wrote:
> Hi Aleksey,
>
>> ht
Hi Aleksey,
> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.05/
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
669 final boolean sized;
670 final boolean exact;
The fields can be declared private.
test/java/lang/String/concat/ImplicitStringConcat
Looks great - thanks!
Maurizio
On 29/11/15 18:23, Aleksey Shipilev wrote:
Hi Maurizio,
Updated webrevs:
http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.04/
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.05/
On 11/27/2015 10:13 PM, Maurizio Cimadamore wrote:
Looks great
Hi Maurizio,
Updated webrevs:
http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.04/
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.05/
On 11/27/2015 10:13 PM, Maurizio Cimadamore wrote:
> Looks great - the only minor quibble is that now StringConcat looks like
> a regular javac
Thanks again, I'll upload the webrevs after further langtools cleanup.
On 11/28/2015 12:05 PM, Andrej Golovnin wrote:
>>> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.04/
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
> 356 ARG,
>
> The comma is n
Hi Aleksey,
>> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.04/
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
356 ARG,
The comma is not needed.
548 // Mock the recipe to reuse the concat generator code
549 StringBuilder sb = n
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 concatKey = new
Context.Key<>()
Hi Andrej,
On 11/27/2015 03:29 PM, Andrej Golovnin wrote:
>> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.03/
>
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
> The inner class Key is final. But the inner classes Recipe and
> RecipeElement are not final. Why?
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
Hi Aleksey,
> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.03/
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
The inner class Key is final. But the inner classes Recipe and
RecipeElement are not final. Why?
The #equals method of the Recipe class has this line:
Hi Ivan,
Thanks for the reviews. Again, I will publish the updated webrevs after
rewiring for Maurizio's comments.
On 11/27/2015 02:33 PM, Ivan Gerasimov wrote:
> 1) As you touch Long.java anyway, could you change
> 508 while (i <= Integer.MIN_VALUE) {
> to
> 508 while (i < Integ
Hi Alexey!
Not a thorough review, but a few of minor comments:
1) As you touch Long.java anyway, could you change
508 while (i <= Integer.MIN_VALUE) {
to
508 while (i < Integer.MIN_VALUE) {
, which may save us a half of nanosecond on some inputs?
And the same on the line# 563.
Hi,
Again, I'll post the updates after addressing another round of
Maurizio's comments :)
On 11/27/2015 10:47 AM, Andrej Golovnin wrote:
>> http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.02/
>
> test/tools/javac/T5024091/T5024091.java
> src/jdk.compiler/share/classes/com/sun/tools/
Hi Aleksey,
> http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.02/
test/tools/javac/T5024091/T5024091.java
src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/StringConcat.java
The files do not have the copyright header.
And I have one stupid question to com.sun.tools.javac.jvm.S
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:
* collect arguments (on a JCAssignOp and on a JCBinary)
* build indy node
Currently you have switches
Hi Maurizio,
Thanks for the reviews!
Updated webrevs:
http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.02/
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.03/
On 11/26/2015 02:40 PM, Maurizio Cimadamore wrote:
> types.boxedClass(syms.voidType).type;
>
> Note that j.l.Void is
On 26/11/15 11:40, Maurizio Cimadamore wrote:
* The code for initializing options in Gen is what we would normally
model with an enum. I.e. you need something like:
enum StringConcatMode {
INLINE,
INDY_PLAIN,
INDY_CONSTANTS;
}
And then put some logic inside the enum class to parse t
Hi Andrej,
Thanks for the review, I'll send the updated webrevs once I address
Maurizio's review comments.
On 11/26/2015 11:42 AM, Andrej Golovnin wrote:
>> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.02/
>
> src/java.base/share/classes/java/lang/Integer.java
> src/java.base/share/clas
Hi Alex,
some comments on the langtools code below:
* please remove the new voidClassType constant from Symtab - and use
this idiom instead:
types.boxedClass(syms.voidType).type;
Note that j.l.Void is always created (see call to
synthetizeBoxTypeIfMissing(voidType) - but there are (or used t
Hi Aleksey,
> http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.02/
src/java.base/share/classes/java/lang/Integer.java
src/java.base/share/classes/java/lang/Long.java
I miss JavaDocs for the returned value of the methods #getChars(int,
int, byte[]) and #getCharsUTF16(int, int, byte[]). And i
On 11/25/2015 03:07 AM, Claes Redestad wrote:
> Some variants might be non-existent in a majority of programs, others
> might not be needed early on, which could help reduce the footprint a bit.
Thanks, indeed, it is savvy to make the MH resolution lazier.
See the update:
http://cr.openjdk.java
Hi Paul,
Thanks for the review!
Updated webrevs:
http://cr.openjdk.java.net/~shade/8085796/webrev.langtools.01/
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.01/
More reviews, please :)
Comments below:
On 11/24/2015 05:16 PM, Paul Sandoz wrote:
>> On 19 Nov 2015, at 21:58, Aleksey S
Hi Aleksey,
in
http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.01/src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java.html
there are a number of internal strategy classes that will set up a
number of MethodHandles in bulk once initialized:
1561 private static f
> On 19 Nov 2015, at 21:58, Aleksey Shipilev
> wrote:
>
> Hi,
>
> I would like to have a code pre-review for Indify String Concat changes.
> For the reference, the rationale, design constraints, etc. are outlined
> in the JEP itself:
> https://bugs.openjdk.java.net/browse/JDK-8085796
>
> The
25 matches
Mail list logo
