RFR (JAXP): 8054196: XPath: support any type
huizhe wang
huizhe.wang at oracle.com
Thu Jan 22 23:04:29 UTC 2015
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
> <mailto: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
>>> <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.
>
> 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/
>> <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>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>> <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>
>
>
>
More information about the core-libs-dev
mailing list