Code review request

[Remi Forax]

On 02/25/2013 04:07 AM, David Holmes wrote:
> On 23/02/2013 8:51 PM, Remi Forax wrote:
>> On 02/21/2013 08:17 PM, Brian Goetz wrote:
>>> At
>>>   http://cr.openjdk.java.net/~briangoetz/jdk-8008670/webrev/
>>>
>>> I've posted a webrev for about half the classes in java.util.stream.
>>> None of these are public classes, so there are no public API issues
>>> here, but plenty of internal API issues, naming issues (ooh, a
>>> bikeshed), and code quality issues.
>>>
>>
>> Hi Brian,
>>
>> All protected fields should not be protected but package visible.
>> Classes are package private so there is no need to use a modifier which
>> offer a wider visibility.
>> The same is true for constructors.
>
> I believe some of these may end up being public (TBD), in which case 
> better to define member accessibility as if they were already public 
> as it greatly simplifies the changes needed later.
>
> David
> -----

Given that the release of jdk9 is at least two years from now, this API 
will change, one will come with a GPU pipeline (Sumatra?) or with a 
flattened bytecode pipeline (my pet project), so trying to figure out 
now what should be public or not is like predicting the future in a 
crystal ball. I think it's better to let all members package private and 
see later.
BTW, I have no problem with protected methods, my main concern is 
protected fields or protected inner classes.

Rémi

>
>
>> For default method, some of them are marked public, some of them are 
>> not,
>> what the coding convention said ?
>>
>> Code convention again, there is a lot of if/else with no curly braces,
>> or only curly braces
>> on the if part but not on the else part.
>> Also, when a if block ends with a return, there is no need to use 
>> 'else',
>>
>> if (result != null) {
>>     foundResult(result);
>>     return result;
>> }
>> else
>>     return null;
>>
>> can be simply written:
>>
>> if (result != null) {
>>     foundResult(result);
>>     return result;
>> }
>> return null;
>>
>>
>> All inner class should not have private constructors, like by example
>> FindOp.FindTask, because
>> the compiler will have to generate a special accessor for them when they
>> are called from
>> the outer class.
>>
>> In AbstractShortCircuitTask:
>> It's not clear that cancel and sharedResult can be accessed directly
>> given that they both have methods that acts as getter and setter.
>> If they can be accessed directly, I think it's better to declare them
>> private and to use getters.
>>
>> Depending on the ops, some of them do nullcheks of arguments at creating
>> time (ForEachOp) , some of them don't (FindOp).
>> In ForEachUntilOp, the 'consumer' is checked but 'until' is not.
>>
>> in ForEachOp, most of the keyword protected are not needed,
>> ForEachUntilOp which inherits from ForEachOp is in the same package.
>>
>> In ForEachUntilOp, the constructor should be private (like for all the
>> other ops).
>>
>> In MatchOp, line 110, I think the compiler bug is fixed now ?
>> The enum MatchKind should not be public and all constructor of all inner
>> classes should not be private.
>>
>> In OpsUtils, some static methods are public and some are not,
>>
>> in Tripwire:
>>    enabled should be in uppercase (ENABLED).
>>    method trip() should be:
>>       public static void trip(Class<?> trippingClass, String msg)
>>
>> cheers,
>> Rémi
>>