Looks good Joe, ship it :-)
On Jan 22, 2015, at 6:04 PM, huizhe wang <huizhe.w...@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.w...@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.w...@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.w...@oracle.com >>>>>>> <mailto:huizhe.w...@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.w...@oracle.com >>>>>>>>> <mailto:huizhe.w...@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.ander...@oracle.com <mailto:lance.ander...@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.ander...@oracle.com <mailto:lance.ander...@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.ander...@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.ander...@oracle.com >> >> >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
