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

[Vicente Romero]

On 10/04/2018 06:53 AM, Maurizio Cimadamore wrote:
> 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?

I see, I was just fixing a typo: EXPLIICT for EXPLICIT
>
> Maurizio

Vicente
>
> 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
>>>
>>
>