Review Request for MVT incubator module

[Maurizio Cimadamore]

On 15/09/17 20:28, mandy chung wrote:
>
>
> On 9/15/17 5:04 AM, Maurizio Cimadamore wrote:
>> Hi Mandy, this looks like a solid piece of work, thanks!
>>
>> Few comments:
>>
>> * I applied all patches and run all relevant tests. Hotspot and JDK 
>> are fine, but langtools is getting some new failures - more 
>> specifically, I get two of those:
>>
>> 1) langtools/test/tools/javac/valhalla/values/CheckNoInvokeDirect.java
>> 2) langtools/test/tools/javac/processing/model/TestSymtabItems.java
>>
> Thanks for catching it.  I left the test run overnight and see 
> TestSymtabItems.java test failed.
>> The failure in (1) seems to be related to some changes you have made 
>> to the CP entries indices. Those changes make the test fail on my 
>> setup - but that has to do with the way I run tests - I always use 
>> the -javacoption:-g option, which sometimes catches interesting 
>> issues (yes, I'm paranoid). TThat said, if the test passes or fails 
>> depending on configuration, it should be @ignored, at least for now - 
>> I've never been too much of a fan of these tests that compare javap 
>> output against some golden files, they are a maintenance nightmare.
>>
> Alternatively, always compile with -g:
> + * @compile -XDenableValueTypes -g CheckNoInvokeDirect.java
true - you can do that too - anyway those tests are half broken, don't 
worry too much about that; thanks for coming up with this trick.
>> The failure in (2) is genuine, and had to do with the fact that the 
>> test is trying to complete a module (jdk.incubator.mvt) too early, 
>> which will cause an assertion error when running the test. I've 
>> discussed a similar problem wityh Jan while ago, and I came up with 
>> the workaround which I have now applied to your case (see attached 
>> patch). With the patch the issue in the test is fixes.
>>
>
> Thanks for the patch.  TestSymtabItems.java passed with the applied 
> patch.
Good
>> * separately, as discussed privately, generally I'm not a big fan of 
>> making high level changes such as turning incubator warnings into 
>> notes in an experimental branch. If we feel that's the way to go, 
>> that change should go into jdk10 and then flow back here. But I 
>> understand that in this case, to make existing tests work in the face 
>> of the unsuppressible warnings, you have to tweak tests to use the 
>> --should-stop internal option - so that means you have to change 
>> tests now, and then back once the enhancement comes from jdk10. So 
>> perhaps, given the warning 2 note change is a one liner, we might be 
>> able to live with it.
> I agree that this issue should be addressed in jdk10.   OK I updated 
> the tests to workaround it with --should-stop:ifError=PARSE that 
> continues with the incubating module and stop before it detects other 
> errors that are not captured in the output.
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/incubator.01/
Looks great - thanks again!

Maurizio
>
> Mandy