Review Request for MVT incubator module
mandy.chung at oracle.com
Fri Sep 15 19:28:18 UTC 2017
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:
More information about the valhalla-dev