RFR (S) 8073550 : java* tools: replace obj.getClass hacks with Assert.checkNonNull or Objects.requireNonNull
Jonathan Gibbons
jonathan.gibbons at oracle.com
Mon Feb 23 16:05:06 UTC 2015
Maurizio,
I will review the revised patch.
Given there is public API involved, has anyone run the corresponding TCK
tests?
-- Jon
On 02/23/2015 06:52 AM, Maurizio Cimadamore wrote:
> The attached path passes all tests. I did a scan over the original
> patch and used Objects.requireNonNull in all API-sensitive
> implementations. I still have some doubts, so I'd appreciated if
> somebody else could glance over the changes...
>
> Maurizio
>
> On 23/02/15 13:29, Maurizio Cimadamore wrote:
>> Hi Aleksey,
>> I did a build/test run on your patch and we have 5 langtools
>> regression failures - they seem to be caused by the fact that there
>> is some infrastructure in either the tests or in the file manager
>> hierarchy itself which assign special semantics to NPEs, so when you
>> replace them with something else (AssertionError), those tests stop
>> working.
>>
>> Here are the failing tests:
>>
>> Execution failed: `main' threw exception: java.lang.AssertionError
>>
>> tools/javac/T6351767.java:
>> javax.tools.JavaCompilerTool.getStandardFileManager().list() treats
>> directories as package
>> tools/javac/api/6410643/T6410643.java: JSR 199: The method
>> JavaCompilerTool.run fails to handle null arguments
>> tools/javac/api/T6400205.java: getClassLoader(location) returns
>> null if getLocation(location) returns null
>> tools/javac/api/T6400207.java: JSR 199: JavaFileManager.list and
>> unset location
>>
>> Execution failed: `main' threw exception:
>> java.lang.reflect.InvocationTargetException
>>
>> tools/javadoc/api/basic/GetTask_OptionsTest.java: javadoc should
>> have a javax.tools.Tool service provider
>>
>>
>> Most of the failing tests show the following stack trace:
>>
>> java.lang.AssertionError
>> at com.sun.tools.javac.util.Assert.error(Assert.java:125)
>> at com.sun.tools.javac.util.Assert.checkNonNull(Assert.java:60)
>> * at com.sun.tools.javac.util.BaseFileManager.nullCheck(BaseFileManager.java:434)*
>> at com.sun.tools.javac.file.JavacFileManager.list(JavacFileManager.java:670)
>> at T6351767.main(T6351767.java:45)
>> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>> at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>> at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>> at java.lang.reflect.Method.invoke(Method.java:498)
>> at com.sun.javatest.regtest.MainAction$SameVMRunnable.run(MainAction.java:776)
>> at java.lang.Thread.run(Thread.java:745)
>>
>> I'm deferring this to Jon as his expertize on this area is much
>> greater than mine. My feeling is that those are public facing
>> methods, whose spec say that an NPE should occur in certain cases, so
>> either we leave the code as is, or we replace it with
>> Objects.requireNonNull (which correctly throws an NPE). Jon, what do
>> you think?
>>
>> Maurizio
>>
>> On 22/02/15 21:22, Maurizio Cimadamore wrote:
>>> Looks great - thanks, I will push it for you.
>>>
>>> Maurizio
>>>
>>> On 20/02/15 14:40, Aleksey Shipilev wrote:
>>>> Hi,
>>>>
>>>> Please review and sponsor the cleanup in langtools code:
>>>> https://bugs.openjdk.java.net/browse/JDK-8073550
>>>> http://cr.openjdk.java.net/~shade/8073550/webrev.01/
>>>>
>>>> It replaces the getClass "hacks" with either proper Asserts, or
>>>> Objects.requireNonNull if Asserts are not available over the module
>>>> boundaries.
>>>>
>>>> Thanks,
>>>> -Aleksey.
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20150223/1c3cb16f/attachment.html>
More information about the compiler-dev
mailing list