RFR: JDK-8211148: javac -source 10 in JDK11 is inconsistent with javac in JDK10

[Maurizio Cimadamore]

Thanks, looks good. As for the test comment, I was referring to this 
hunk of the patch:

-        EXPLIICT_SIMPLE("A", ExplicitKind.EXPLICIT),
-        EXPLIICT_SIMPLE_ARR1("A[]", ExplicitKind.EXPLICIT),
-        EXPLIICT_SIMPLE_ARR2("A[][]", ExplicitKind.EXPLICIT),
+        EXPLICIT_SIMPLE("A", ExplicitKind.EXPLICIT),
+        EXPLICIT_SIMPLE_ARR1("A[]", ExplicitKind.EXPLICIT),
+        EXPLICIT_SIMPLE_ARR2("A[][]", ExplicitKind.EXPLICIT),


Looks like the contents are the same, what's up?

Maurizio

On 03/10/18 17:39, Vicente Romero wrote:
> Hi Maurizio,
>
> Thanks for the review. I have updated the webrev [1]
>
> [1] http://cr.openjdk.java.net/~vromero/8211148/webrev.01/jdk.dev.patch
>
> some comments below.
>
> On 10/03/2018 06:18 AM, Maurizio Cimadamore wrote:
>> The changes look generally good, but I have some questions/comments.
>>
>> * (cosmetic, optional) This change:
>>
>> if (Feature.VAR_SYNTAX_IMPLICIT_LAMBDAS.allowedInSource(source)) {
>> +                        log.error(DiagnosticFlag.SYNTAX, param.pos, 
>> Errors.VarNotAllowedArray);
>> +                    } else {
>> +                        log.error(DiagnosticFlag.SYNTAX, param.pos, 
>> Errors.VarNotAllowedHere);
>> +                    }
>>
>> Could be slightly simplified as:
>>
>> log.error(DiagnosticFlag.SYNTAX, param.pos, 
>> Feature.VAR_SYNTAX_IMPLICIT_LAMBDAS.allowedInSource(source)
>>  ? Errors.VarNotAllowedArray : Errors.VarNotAllowedHere);
>
> done it
>>
>>
>> * A more serious issue is, I think that the code here:
>>
>> if (Feature.VAR_SYNTAX_IMPLICIT_LAMBDAS.allowedInSource(source)) {
>> +                        param.startPos = 
>> TreeInfo.getStartPos(param.vartype);
>> +                        param.vartype = null;
>> +                    } else {
>> +                        log.error(DiagnosticFlag.SYNTAX, param.pos, 
>> Errors.VarNotAllowedHere);
>> +                    }
>>
>> Should really call the parser method 'checkSourceLevel' which will 
>> issue the correct error if the feature is not allowed (e.g. 'var in 
>> lambda not allowed, use source 11 to enable'). Which brings to:
>>
>> * You should define a compiler key for the new feature (e.g. 'var in 
>> lambda') so that the framework will pick that up when generating a 
>> diagnostic using the above method
>
> done too,
>
>>
>> * In the classification logic, I believe the goal of the changes is 
>> to disable the diagnostic generation for illegal combinations of 
>> lambda parameter declarations when one or more of these involve a 
>> 'var' which would have already been flagged in a separate diagnostic. 
>> That is, these changes are to avoid to report duplicate and spurious 
>> diagnostics, right?
>
> yep, I reorganized the matrix putting the var rows and columns to the 
> top and to the left, there wasn't a real reason to to this, but I 
> thought that it could help reading the code in the future. Probably 
> very subjective but still :) then yes if the source doesn't allow var 
> in implicit lambdas a `mask` is applied to the value read from the 
> matrix effectively changing it to a null to avoid duplicate reports.
>>
>> * In the LambdaParserTest I see no relevant changes, but still the 
>> patch shows something - is there some trailing space or indent 
>> different at play?
>
> the change here was to add a case for which errors are expected when 
> var is used and source is 10
>>
>> Maurizio
>
> Vicente
>
>>
>> On 03/10/18 01:40, Vicente Romero wrote:
>>> Hi,
>>>
>>> Please review the fix for [1] at [2]. There were a number of places 
>>> where the compiler was not checking if var syntax for implicit 
>>> lambdas was allowed or not. This implied that the JDK11 compiler was 
>>> accepting code with this feature even if the -source 10 option was 
>>> passed, this shouldn't happen. This patch fixes this and issues the: 
>>> "var not allowed here" error message as expected.
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8211148
>>> [2] http://cr.openjdk.java.net/~vromero/8211148/webrev.00/jdk.dev.patch
>>
>