Review Request for MVT incubator module

[mandy chung]

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
> 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.
> * 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/

Mandy