On 07/29/2013 09:06 PM, Brian Burkhalter wrote:
Joe / Chris:
The updated webrev is in the same place,
http://cr.openjdk.java.net/~bpb/8020539/
If this looks good I will need an approval.
It looks ok to me.
-Chris.
On Jul 29, 2013, at 12:05 AM, Joe Darcy wrote:
I recommend a few changes here:
* For readability, rename "NonASCIIDigit" as "NonAsciiDigit"
Changed.
* Since Scanner is in the regex package, it may be more appropriate to
stick more of a regex notation than an ebnf notation, but keeping the
definition list structure tags.
Changed back to regex notation.
Offhand, I wasn't aware Dequeue and Queue were maintained in Doug's
CVS, but I agree changes to those files should flow through there first.
Deque and Queue removed from the webrev:
http://cr.openjdk.java.net/~bpb/8020539/
Otherwise, the changes look fine. If you like, the non-Scanner changes
can be split out and pushed first.
I've left it as one changeset.
On Jul 29, 2013, at 2:43 AM, Chris Hegarty wrote:
Looks fine ( I did not go through the Scanner changes ).
Some minor comments:
1) List
Trivially I wonder why you decided to go with '< '>', rather
than <pre>{@code ... }</pre>, which I think is more readable.
Changed.
2) Map
Trivially, I would keep the @param's order the same as the actual
method parameters order.
Changed.
3) Optional
Strangely, there doesn't appear to be a '@param' for the class level
type T. doclint is not warning about this either.
Not changed.
Thanks,
Brian
