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