RFR (JAXP): 8054196: XPath: support any type

huizhe wang huizhe.wang at oracle.com
Thu Jan 22 05:18:02 UTC 2015 

On 1/21/2015 5:09 PM, Lance Andersen wrote:
> Hi Joe,
>
> I think this is OK (as we discussed offline), one minor 
> comment/suggestion below
>
> Best
> Lance
> On Jan 21, 2015, at 7:45 PM, huizhe wang <huizhe.wang at oracle.com 
> <mailto:huizhe.wang at oracle.com>> wrote:
>
>> Thanks Lance for pointing me to Joe's -Xlint:all email thread. I 
>> re-compiled with -Xlint:all and noticed 7 warnings in this webrev. 
>> I've fixed them with a new webrev:
>> New webrev: http://cr.openjdk.java.net/~joehw/jdk9/8054196/webrev01/ 
>> <http://cr.openjdk.java.net/%7Ejoehw/jdk9/8054196/webrev01/>
>>
>> Below are the details.
>> Refer to the previous webrev: 
>> http://cr.openjdk.java.net/~joehw/jdk9/8054196/webrev/ 
>> <http://cr.openjdk.java.net/%7Ejoehw/jdk9/8054196/webrev/>
>>
>> 1. XPathExpressionImpl.java:143 and XPathImpl.java:213
>>
>>   These warnings are fixed by having the getXPathResult method in 
>> XPathImpUtil returning the type instead
>>
>> 2. XPathExpressionImpl.java:165: warning: [rawtypes] found raw type: 
>> XPathEvaluationResult
>>    similarly, XPathImpl.java:222 and XPathImpl.java:235
>>
>>    Changed to return XPathEvaluationResult<?>
>>
>> 3. XPath.java:359, XPath.java:454, XPathExpression.java:252 and 
>> XPathExpression.java:344
>>   These are warnings from the default methods, casting the result of 
>> existing (old) methods. Use type.cast instead.
>>
>> 4. XPathNodesImpl.java:97: warning: [cast] redundant cast to Node
>>    Removed the cast
>>
>> 5. XPathResultImpl.java:160: warning: [fallthrough] possible 
>> fall-through into case
>>    Added break;
>
> I would add a comment or  probably 
> use @SuppressWarnings("fallthrough") instead

Thanks!  As I look at adding a comment, I realized I needed to be 
explicit on the numeric types as the spec stated. I've replaced the 
abstract Number type in XPathImplUtil::isSupportedClassType with 
Double/Integer/Long, and also in XPathResultImpl::getValue.

I also added a test case for Long in XPathAnyTypeTest::test05, and a new 
test test06 to verify that the numeric types other than the supported 
can not be used as the type.

http://cr.openjdk.java.net/~joehw/jdk9/8054196/webrev01/ 
<http://cr.openjdk.java.net/%7Ejoehw/jdk9/8054196/webrev01/>

Thanks,
Joe

>>
>> Thanks,
>> Joe
>>
>> On 1/20/2015 10:51 AM, huizhe wang wrote:
>>>
>>> On 1/20/2015 7:02 AM, Lance Andersen wrote:
>>>> Hi Joe,
>>>>
>>>> I think the changes look fine.
>>>
>>> Thanks.
>>>>
>>>> I am wondering if we have any suggested standard for the use of 
>>>> @Override  as I see it is inconsistent in its usage with methods 
>>>> implementing an interface.  Is this something we should add to 
>>>> existing code?  I don't see Netbeans asking for it to be done 
>>>> except in the case of an overridden method?
>>>
>>> NetBeans does make a suggestion such as: "Add @Override Annotation". 
>>>  I think you're right about avoiding comment duplication. In case of 
>>> existing "evaluate" methods, I previously updated* the javadocs for 
>>> both the interface and impl. It makes sense to take advantage of the 
>>> "automatically inherit" feature 
>>> <http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javadoc.html#inheritingcomments> 
>>> of the javadoc tool to avoid the duplication.
>>>
>>> *Note that the update was only on format and styles, and some 
>>> re-wording, no changes on definition or semantics.
>>>
>>>>
>>>> Thank you for the extra clean up Joe
>>>
>>> Thank you, now it looks better and cleaner.
>>>
>>> Best,
>>> Joe
>>>
>>>>
>>>> Best
>>>> Lance
>>>> On Jan 16, 2015, at 9:33 PM, huizhe wang <huizhe.wang at oracle.com 
>>>> <mailto:huizhe.wang at oracle.com> <mailto:huizhe.wang at oracle.com>> wrote:
>>>>
>>>>>
>>>>> On 1/16/2015 1:29 PM, Lance Andersen wrote:
>>>>>> Hi Joe,
>>>>>>
>>>>>>
>>>>>>
>>>>>> Overall it is OK, a few minor comments
>>>>>>
>>>>>> -  Is there a reason that XPathExpressionImpl  is no longer 
>>>>>> public and XPathImpl is public?
>>>>>
>>>>> Ok, I'll keep both public, may be useful in the future.
>>>>>
>>>>>> - I think you can leverage {@inheritdoc} in your impl classes to 
>>>>>> avoid comment duplication possibly?
>>>>>
>>>>> Looks like javadoc "Automatically inherit comment ". So @Override 
>>>>> is good enough. I've removed the javadocs for methods that 
>>>>> override, including the existing methods. It's good to avoid the 
>>>>> duplication, and potential copy n paste errors.
>>>>>
>>>>>> - please remember to make the copyright year 2015
>>>>>
>>>>> Done.
>>>>>
>>>>>> - XPathTestBase, can the static block be moved to a @BeforeClass
>>>>>
>>>>> Moved to within the Dataprovider.
>>>>>
>>>>>> - XPathNodes, should that have an @since 1.9?
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> - Given you are not including @param, etc for your test comments, 
>>>>>> you might want to consider /* */ vs /** */ style comments.
>>>>>
>>>>> Looks like s/\/**/\/* worked the trick.
>>>>>
>>>>>> That is consider if you really want doc comments (really a style 
>>>>>> choice but some IDEs will issue a warning for missing tags
>>>>>
>>>>> Yeh, it's good to turn off the warning "light" :-)
>>>>>
>>>>> http://cr.openjdk.java.net/~joehw/jdk9/8054196/webrev/ 
>>>>> <http://cr.openjdk.java.net/%7Ejoehw/jdk9/8054196/webrev/>
>>>>>
>>>>> Thanks,
>>>>> Joe
>>>>>
>>>>>>
>>>>>>
>>>>>> HTH
>>>>>>
>>>>>> Best,
>>>>>> Lance
>>>>>> On Jan 16, 2015, at 2:51 PM, huizhe wang <huizhe.wang at oracle.com 
>>>>>> <mailto:huizhe.wang at oracle.com>> wrote:
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Could you review the change?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Joe
>>>>>>>
>>>>>>> On 12/18/2014 1:24 PM, huizhe wang wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> This is to add support for any type and improvement with new 
>>>>>>>> features reflected in the new evaluateExpression methods, 
>>>>>>>> XPathEvaluationResult and XPathNodes.
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8054196
>>>>>>>> http://cr.openjdk.java.net/~joehw/jdk9/8054196/webrev/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Joe
>>>>>>>
>>>>>>
>>>>>> <Mail Attachment.gif> 
>>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> 
>>>>>>
>>>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
>>>>>> Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>>>>> Oracle Java Engineering
>>>>>> 1 Network Drive
>>>>>> Burlington, MA 01803
>>>>>> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> 
>>>>
>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
>>>> Andersen| Principal Member of Technical Staff | +1.781.442.2037
>>>> Oracle Java Engineering
>>>> 1 Network Drive
>>>> Burlington, MA 01803
>>>> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com> 
>>>> <mailto:Lance.Andersen at oracle.com>
>>>>
>>>>
>>>>
>>>
>>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>

More information about the core-libs-dev mailing list