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

Thu Jan 22 01:09:08 UTC 2015

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,
> 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>
>>> 
>>> 
>>> 
>> 
> 

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