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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Feb 26 00:25:07 UTC 2015


On 25/02/15 23:48, Jonathan Gibbons wrote:
> There are a number of minor issues here. I'm reviewing the 
> langtools-v2.patch file.
>
> 1. Perhaps because of the churn, a number of files now have a new 
> unnecessary import.
I'll clean those up
>
> 2. A call of either Assert.checkNonNull(x); or 
> Objects.requireNonNull(x); does not need an explanatory comment of "// 
> null check" and such comments should be removed.  I can understand why 
> such comments might have been left by automated scripts; I don't 
> understand why in a few places, such comments were newly added.
I was the one adding them - out of an attempt to make it explicit as to 
which checks were 'demanded' by the API. I can remove them no problem.
>
> 3. In general, we should not depend on the javac internal Assert 
> mechanism outside of javac.
Uh - ok. Not sure I fully buy this - i.e. javadoc reuses 99% of javac so 
I'm not sure what buys us not to use Assert mechanism there...
>
> 4. In some places where there are multiple checks, the old code used 
> to do all the checks before using any of the checked values. That has 
> been changed in a few places, and in one place could leave an open 
> stream lying around. It would be better to preserve the original 
> execution order.
Ouch - didn't spot those, thx.

I will address the comments and post another webrev with a delta patch.

Maurizio
>
>
> Here is the full list of comments:
>
> DocTreePath
>     redundant import of Assert
> 102,103 redundant comments
>
> TreePath
>     redundant import of Assert
> 62,63   redundant comments
>
> com.sun.tools.classfile.ClassReader
>     com.sun.tools.classfile.* should not really depend on anything in
>         com.sun.tools.javac.*
>     In this case, I think Objects.requireNonNull is preferable
>     better to do the check before opening input stream
>
> com.sun.tools.classfile.ClassFileDependencies
>     com.sun.tools.classfile.* should not really depend on anything in
>         com.sun.tools.javac.*
>     In this case, I think Objects.requireNonNull is preferable
>
> ClientCodeWrapper
>     NPE would be better than an assert failure, so Objects.checkNonNull
>     would be better
>
> JavacScope
>     redundant import, redundant comment
>     in this instance AssertionError would be better; this is an
>     internal method, and javac would be wrong to provide null here
>
> JavacTaskImpl
>     redundant comment
>
> JavacTool, JavacFileManager, Locations, RegularFileObject
>     redundant import, redundant comments
>
> ZipArchive
>     redundant import
>
> PathFileObject
>     redundant import, redundant comments
>
> BaseFileManager
> 434-436 remove (inline) unnecessary method
>
> JavahTask
>     redundant imports
>
> AttributeWriter
>     Previously, the code did two checks, and only if both are OK
>     did the assigments happen. With the proposed check, one
>     assignment might happen, but not the other.
>
> JavapTask
>     redundant imports, redundant comment
>
> Content
>     doclet internal class should not use javac internal class
>     better to use Objects.requireNonNull, or remove (inline)
>     the method
>
> Start
>     doclet internal class should not use javac internal class
>     better to use Objects.requireNonNull
>
> JavadocTool
>     duplicate imports
>
>
> -----------------------------
>
> -- 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/20150226/b76a93c8/attachment.html>


More information about the compiler-dev mailing list