RFR (S) 8073550 : java* tools: replace obj.getClass hacks with Assert.checkNonNull or Objects.requireNonNull

[Maurizio Cimadamore]

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/0f78bdc3/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: langtools-v2.patch
Type: text/x-patch
Size: 31968 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20150223/0f78bdc3/langtools-v2-0001.patch>