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

Lance Andersen lance.andersen at oracle.com
Fri Jan 23 19:04:50 UTC 2015 

Looks good Joe, ship it :-)


On Jan 22, 2015, at 6:04 PM, huizhe wang <huizhe.wang at oracle.com> wrote:

> Thanks Lance.
> 
> On 1/22/2015 9:11 AM, Lance Andersen wrote:
>> Hi Joe,
>> 
>> I just looked at the changes below,
>> 
>> I looked at the changes below… see minor comments
>> On Jan 22, 2015, at 12:18 AM, huizhe wang <huizhe.wang at oracle.com> wrote:
>> 
>>> 
>>> 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> 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/
>>>>> 
>>>>> Below are the details.
>>>>> Refer to the previous webrev: http://cr.openjdk.java.net/~joehw/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.
>> 
>> Can we document why XPathImplUti.getXPathResult() should return null, same for XPathResultImpl.getValue()
> 
> Done.
> 
> While I was there, I also removed the 'type' parameter from the constructor of XPathNodesImpl and then XPathResultImpl::classToType method as it was only used to for constructing the XPathNodes object. 
>>> 
>>> 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.
>> This looks good
> 
> http://cr.openjdk.java.net/~joehw/jdk9/8054196/webrev02/
> 
> Thanks,
> Joe
> 
>>> 
>>> http://cr.openjdk.java.net/~joehw/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>> 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/
>>>>>>>> 
>>>>>>>> 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>
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>>> <Mail Attachment.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
>>>> 
>>>> 
>>>> 
>>> 
>> 
>> <Mail Attachment.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
>> 
>> 
>> 
> 



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

More information about the core-libs-dev mailing list