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() > > 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/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 >> >> >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
