[OpenJDK 2D-Dev] [9] Review Request: 8042199 The build of J2DBench via makefile is broken after the JDK-8005402

[Andrew Brygin]

Hello Sergey,

  the change looks fine to me.

Thanks,
Andrew

On 8/13/2014 4:31 AM, Sergey Bylokhov wrote:
> Hi Jim.
> Yes, you are right, I missed it even after attentive viewing.
> Typo was fixed:
> http://cr.openjdk.java.net/~serb/8042199/webrev.03
>
>> Hi Sergey,
>>
>> I understand that the type was changed for a reason, but the variable is
>> spelled "Platfrom" - which is not a word - and the same text appears in
>> the comment there.
>>
>> The word intended there is, I believe, "Platform"...
>
>
> On 8/12/14 4:20 PM, Sergey Bylokhov wrote:
>> Hi, Jim.
>> Actually a Boolean was changed to a boolean, to skip autoboxing.
>> http://cr.openjdk.java.net/~serb/8042199/webrev.02/src/share/demo/java2d/J2DBench/src/j2dbench/tests/cmm/CMMTests.java.sdiff.html
>>
>>>> The new Readme explanation looks good.  Thanks for updating the new code
>>>> for pre-1.5.
>>>> I notice that one of the changes (in CMMTests) is to a line with a typo
>>>> (Platfrom instead of Platform both in the code and in the comment on the
>>>> same line), but fixing the typo might affect a lot of other lines...?
>> 			...jim
>>
>> On 8/12/14 8:32 AM, Sergey Bylokhov wrote:
>>> On 12.08.2014 1:34, Jim Graham wrote:
>>>> Hi Sergey,
>>>>
>>>> Is the -g:none the result of #2 below?
>>> This was changed to align with <javac debug="flase"...>  in build
>>> xml(typo was fixed as well).
>>>> Also, if I read the email trail correctly then source/target=1.6 is
>>>> only there because JDK 9 compiler doesn't let you request anything
>>>> earlier. The Readme doesn't mention this and it should.
>>> done.
>>>> Also, I'm not sure why it says that it requires at least 1.5 instead
>>>> of 1.4 now as there is no mention of any code changes that don't work
>>>> on 1.4 any more - were there?  The only explanation I saw below was
>>>> the source/target specs allowed by the 9 compiler, not any results of
>>>> trying to compile it on 1.4 or 1.5...
>>> The reason was in the some java features(@overried/enums) in the new
>>> colors management tests from JDK-8005402. In the last version I fix
>>> that, and now we can compile the tests using jdk 1.4.2. Note that 1.3 is
>>> not supported, because the new tests uses some api which was added in 1.4.
>>>
>>> The new version of the fix:
>>> http://cr.openjdk.java.net/~serb/8042199/webrev.02
>>>
>>>> So, the Readme should minimally mention that source/target is set to
>>>> 1.6 in the makefile only because of support in the 9 compiler, and we
>>>> should double check which compilers it actually is still buildable on
>>>> and record that in the Readme.  (Again, maybe I missed the part where
>>>> we tried it on 1.4 and failed, but it works on 1.5 - that wasn't
>>>> included below...)
>>>>
>>>>           ...jim
>>>>
>>>> On 8/11/14 9:01 AM, Sergey Bylokhov wrote:
>>>>> Hello.
>>>>> Please review the new version of the fix:
>>>>> http://cr.openjdk.java.net/~serb/8042199/webrev.01
>>>>>     - target&source changed to 1.6. But readme still mentions that
>>>>> benchmark requires at least jdk1.5 to compile.
>>>>>     - I found mismatch between ant/make about debug information. fixed
>>>>>     - the fix for 8005402 did not properly update makefiles for images.
>>>>> fixed
>>>>>     - dest was changed to dist, because this is default location of
>>>>> J2DBench.jar.
>>>>>
>>>>> On 07.08.2014 23:55, Jim Graham wrote:
>>>>>> The only intention was that we be able to compare against older
>>>>>> runtimes when needed.  We could ask whether we care about how we
>>>>>> currently compare against 1.2 any more...?  If we're really that
>>>>>> curious, we can either change the target and compile with an older
>>>>>> compiler, or find an older version of it (but that would be a little
>>>>>> apples-to-oranges).  In any case, we'd have options for doing it even
>>>>>> if they weren't as convenient as just running it on an older jvm.
>>>>>>
>>>>>> It's "convenience and need" vs. "what's possible" and right now "need"
>>>>>> is probably a very small value (for <1.5) and "what's possible" just
>>>>>> changed...
>>>>>>
>>>>>>               ...jim
>>>>>>
>>>>>> On 8/7/14 11:31 AM, Phil Race wrote:
>>>>>>> Perhaps we have to make that the default but add a comment that since
>>>>>>> this
>>>>>>> is bundled with JDK 9 it must use at least a 1.6 target but the
>>>>>>> intention is
>>>>>>> that it be able to be compiled with and targeted to, earlier JDKs
>>>>>>>
>>>>>>> BTW I guess dest->dist is OK but I imagine Jim really did mean "dest"
>>>>>>>
>>>>>>> -#> java -jar dest/J2DBench.jar -batch -loadopts options/default.opt \
>>>>>>> +#> java -jar dist/J2DBench.jar -batch -loadopts options/default.opt \
>>>>>>>
>>>>>>>
>>>>>>> -phil.
>>>>>>>
>>>>>>> On 8/7/2014 9:23 AM, Sergey Bylokhov wrote:
>>>>>>>> Hello, Phil.
>>>>>>>> jdk 9 now supports "-target 1.6+/-source 1.6+" options only. Looks
>>>>>>>> like we should use this minimum version too.
>>>>>>>> If you have no objections I'll prepare the new version of the fix
>>>>>>>>
>>>>>>>> On 14.05.2014 21:09, Phil Race wrote:
>>>>>>>>> Hmm .. the thing here is that I know that Jim was very keen that
>>>>>>>>> this
>>>>>>>>> benchmark always be runnable on JDK 1.2
>>>>>>>>> Deleting the comment to that effect and setting source level to 1.5
>>>>>>>>> goes against this.
>>>>>>>>> What is the rationale, and why can't it be reverted to be able to
>>>>>>>>> build on 1.4 and run
>>>>>>>>> on 1.2 ? If it uses JDK 1.5 language features, just back them
>>>>>>>>> out. If
>>>>>>>>> it uses JDK 1.5
>>>>>>>>> APIs then maybe Jim had to handle something similar and has an
>>>>>>>>> idea ?
>>>>>>>>>
>>>>>>>>> -phil.
>>>>>>>>>
>>>>>>>>> On 4/30/2014 4:13 AM, Andrew Brygin wrote:
>>>>>>>>>> Hello Sergey,
>>>>>>>>>>
>>>>>>>>>>    the change looks fine to me.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Andrew
>>>>>>>>>>
>>>>>>>>>> On 4/30/2014 3:12 PM, Sergey Bylokhov wrote:
>>>>>>>>>>> Hello.
>>>>>>>>>>> Please review the small fix for jdk 9.
>>>>>>>>>>> Makefile and README were fixed.
>>>>>>>>>>>
>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8042199
>>>>>>>>>>> Webrev can be found at:
>>>>>>>>>>> http://cr.openjdk.java.net/~serb/8042199/webrev.00
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Best regards, Sergey.
>>>>>>>>
>>>>>
>>>