RFR: 8174735 Update JAX-WS RI integration to latest version

Roman Grigoriadi roman.grigoriadi at oracle.com
Thu Feb 16 14:25:16 UTC 2017


Hi Daniel,


On 02/16/2017 11:39 AM, Daniel Fuchs wrote:
> Hi Aleksej,
>
> On 15/02/17 23:49, Aleks Efimov wrote:
>> Hi,
>>
>> The new webrev with addressed comments was uploaded here:
>> http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01
>
> This is probably a question for the upstream project, but I'm
> puzzled by this change:
>
> http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/jdk.xml.bind/share/classes/com/sun/codemodel/internal/JModuleDirective.java.frames.html 
>
>
> ... and even more since Integer.hashCode() calls Integer.hashCode(int) 
> which just returns the value of the integer...
> What's the purpose of calling valueOf which 1. contradict the javadoc
> comment and 2. might trigger the creation of a new Integer instance?
Thanks for pointing out, we have removed both Integer#valueOf and 
Integer#hashCode calls, it doesn't make any sense to be there. This 
puzzling line resulted from useless Ingeger#hashCode(...) call which got 
refactored to be target 1.7 compatible.
>
>
> Nit: There are also some files which present some strange
>      formatting/misalignment where new code/comment was added.
>      I have noted them below:
>
> There's strange formatting for the anonymous subclass
> of AbstractList in this file:
>
> http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/packaging/mime/internet/InternetHeaders.java.frames.html 
>
>
>  324             headerValueView = new AbstractList<String>() {
>  325                 @Override
>  326                                 public String get(int index) {
>  327                     return headers.get(index).line;
>  328                 }
>  329
>  330                 @Override
>  331                                 public int size() {
>  332                     return headers.size();
>  333                 }
>  334             };
>
> and
> http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/packaging/mime/internet/MimePartDataSource.java.frames.html 
>
>
> has also some alignment issues where @Overrides were added.
>
> The following file has some comment alignment issues which impair
> readability:
>
> http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/01/jaxws/src/java.xml.ws/share/classes/com/sun/xml/internal/messaging/saaj/packaging/mime/util/ASCIIUtility.java.frames.html 
>
I have fixed the alignment in SAAJ.
Both changes will get to JDK next sync.
>
> best regards,
Thanks for comments,
Roman
>
> -- daniel
>
>>
>> Best,
>> Aleksej
>>
>>
>> On 15/02/17 15:42, Roman Grigoriadi wrote:
>>> Hi Mandy,
>>>
>>> On 02/14/2017 11:53 PM, Mandy Chung wrote:
>>>>> On Feb 14, 2017, at 4:00 AM, Roman Grigoriadi
>>>>> <roman.grigoriadi at oracle.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please review standalone JAXB/JAXWS changes, synced to jdk/jaxws 
>>>>> repo.
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8174735
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~aefimov/jaxws-integrations/8174735/00/
>>>>
>>>> jaxws/src/jdk.xml.ws/share/classes/com/sun/tools/internal/ws/wscompile/WsimportTool.java 
>>>>
>>>>
>>>>
>>>> -    /** JAXWS module name. JAXWS dependency is mandatory in
>>>> generated Java module. */
>>>> -    private static final String JAXWS_MODULE = "java.xml.ws";
>>>> +    /** JAXB module name. JAXB dependency is mandatory in generated
>>>> Java module. */
>>>> +    private static final String JAXWS_PACKAGE = "java.xml.ws”;
>>> this looks to be merge failure on our side, will fix it again.
>>>>
>>>> JAXWS_MODULE is the right name as we discussed in the last JAX-WS
>>>> integration to jdk9. This change should be reverted and the upstream
>>>> project  should be fixed.
>>>>
>>>> +                    if ("-release".equals(opt) && 9 >=
>>>> getVersion(javacOptions.get(i + 1))) {
>>> thanks, will be fixed to --release
>>>>
>>>> javac supports `—-release` (double dashes, GNU long form style) but
>>>> not the single dash option.  Is this new option in wsimport and wsgen
>>>> tools?  It should probably be consistent with javac.
>>>>
>>>> You can run jdeps —-check java.base,java.xml option to double check
>>>> if any remaining qualified exports to these modules.
>>>>
>>>> Otherwise looks okay.
>>>>
>>>> Mandy
>>>>
>>>
>>
>



More information about the core-libs-dev mailing list