Hi Chris,
Thanks for the review.
On 11/11/15 6:17 AM, Chris Hegarty wrote:
On 10 Nov 2015, at 18:55, Roger Riggs <roger.ri...@oracle.com> wrote:
A few of the proposed replacements of ?: with requireNonNullElse were unsuitable
because in the particular context, null is an allowed replacement value.
The webrev has been updated to revert changes:
- two uses in
jdk/src/java.base/share/classes/java/time/format/DateTimePrintContext.java
(Line 145 and 160)
- src/java.base/share/classes/java/time/temporal/TemporalQueries.java -
reverted all changes
webrev:
http://cr.openjdk.java.net/~rriggs/webrev-require-non-null-8141652/
Overall looks good.
I’m not sure it is worth adding an instance field, zeroAliases, to Charset?
As for the aliases array, the impact seemed smaller to create a constant
empty String[]
than to trigger the lambda mechanisms and be creating a new empty
String[] every time.
Plan B would probably be to revert to the previous code.
Thanks, Roger
-Chris.
Thanks, Roger
p.s. the previous webrev is renamed:
http://cr.openjdk.java.net/~rriggs/webrev-require-non-null-8141652-v1/
On 11/9/2015 2:35 PM, Roger Riggs wrote:
Hi Stephen,
On 11/6/2015 10:42 PM, Stephen Colebourne wrote:
Seems fine to me.
I would have inlined the ZoneId change to one line:
String id = Objects.requireNonNullElse(aliasMap.get(zoneId), zoneId);
to avoid the local variable change, but no big deal.
Stephen
Thanks, I'll fix that.
Roger
On 6 November 2015 at 20:24, Roger Riggs <roger.ri...@oracle.com> wrote:
Please review the renaming of java.util.Object methods, the new method names
are:
public static <T> T requireNonNullElse(T obj, T defaultObj);
public static <T> T requireNonNullElseGet(T obj, Supplier<? extends T>
supplier);
The only remaining possible gotcha for developers is the requireNonNull(T,
String)
method that might be confused with requireNonNullElse(T, T). The longer
name is probably the one you want.
If you see a value that looks like an exception string, you've got the wrong
method.
To validate some of the uses, I visually scanned src/java.base looking
expressions of the pattern (e1 != null) ? e1 : e2
Most uses of the ternary operator involve either primitive types or
conditions other than != null;
quite a few involved returning null and so were not appropriate to be
converted.
10 files contained relevant expressions:
- 2 should be using Objects.toString() to provide a default value for a
String.
- 2 can use the requireNonNullElseGet form to delay evaluating the
alternative value.
(2 others were too early in the boot cycle to use lambda).
- The rest followed the expected pattern; though checking if throwing an
exception was extra work.
Please review and comment.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-require-non-null-8141652/
Thanks for the detailed evaluation and discussion.
Roger
