Review Request for MVT incubator module
maurizio.cimadamore at oracle.com
Fri Sep 15 20:28:26 UTC 2017
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
>> * 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:
Looks great - thanks again!
More information about the valhalla-dev