Fwd: Re: RFR: JEP 359-Records: compiler code

[Vicente Romero]

Hi Joe,

Thanks for the review,

On 11/4/19 12:33 AM, Joe Darcy wrote:
> Hi Vicente,
>
> I've taken a pass over the javax.lang.model portions of the change.
>
> Elements.recordComponentFor should have an @implSpec tag.
>
> I suggest a double-check of the existing visitors/scanners that used 
> to extend one of the "FooVisitor9" types and was updated to extend 
> "FooVisitor14". In prior work, I had to add a new method override for 
> visitRecordComponent to the printing processor to get the proper 
> semantics of that class. For example, PubapiVisitor might need an update.

I have updated the code for all the comments but the one above. I don't 
understand what you are exactly asking for,

>
> Also, please add the new "FooVisitor14" classes for types.
>
> In terms of how the tests are placed, I'd prefer to see tests of, say 
> annotation processing of records, groups with the existing annotation 
> processing tests as opposed to grouped with pure records tests. I 
> think this arrangement provides better test discoverability long term; 
> for example, if someone is updating annotation processing and wants to 
> make sure all the annotation processing tests are run.
>
> I recommend the annotation processing tests more consistently subclass 
> JavaTestingAbstractProcessor as opposed to directly subclassing 
> AbstractProcessor.
>
>
> In TestRecord.java, please add newlines to the string arguments to the 
> options method on line 146.
>
> Thanks,
>
> -Joe

Thanks,
Vicente

>
> On 10/31/2019 9:11 PM, Vicente Romero wrote:
>> Hi all,
>>
>> Thanks for all the feedback so far. I have published another 
>> iteration at [1]. New in this iteration:
>>
>> Flags.java:
>>
>> - I have sync the javadoc and the comment on the RECORD flag
>> - renamed MemberRecordClassFlags -> MemberRecordFlags
>>
>> com.sun.tools.javac.code.Kinds.java:
>> - in order to generate better error messages I have added two new 
>> kinds: RECORD_COMPONENT and RECORD
>>
>> Symbols.java
>> - moved method isRecord to ClassSymbol for the rest of the symbols 
>> moved back to using the bitwise test
>> - removed method ::isDynamic from MethodSymbol
>>
>> Attr.java
>> - implemented a spec change to relax that constraint that every 
>> non-canonical constructor should call the canonical constructor. Now 
>> the spec forces every non-canonical constructor to invoke a different 
>> constructor as its first statement. This actually implied removing 
>> some code.
>>
>> Check.java
>> - at method ::duplicateError, there is new code to avoid generating 
>> two errors when the user declares a record with duplicated record 
>> component names. If this happens, then a compiler generated canonical 
>> constructor would have duplicated parameters. This code is preventing 
>> the compiler to generate a second error referring to a method that is 
>> not visible to the user. Another option is to not generate the 
>> canonical constructor at all if there are duplicate record components
>> - at method ::checkUnique, there is new code to improve the error 
>> message when the user defines a constructor with the same erasure as 
>> the canonical constructor. Still the error message is not perfect 
>> because it shows in both cases the erasure of the methods, so they 
>> seem to be the same. But this is not related to records code, that's 
>> a simplification that happens at the moment of reporting the error.
>>
>> TypeEnter
>> - removed some commented code
>> - ::addRecordMembersIfNeeded here I added new code to avoid 
>> generating accessor methods for forbidden record component names. 
>> This is to avoid spurious errors later as any way the compiler will 
>> complain because of the forbidden record component names. The issue 
>> with this solution is that the list of forbidden record component 
>> names has been duplicated here. The other location is Attr. We can 
>> probably find a common place to define it. Probably a new utility class?
>>
>> JavacParser:
>> - fixed the bug that the compiler was not allowing final local records
>> - removed the check for abstract modifiers in records, this code was 
>> redundant as Check was already checking this
>> - removed the check for duplicated record component names
>> - added a check to forbid instance initializers in records
>>
>> compiler.properties
>> - I hope I got the error messages right, I tried to reflect all the 
>> comments, suggestions, etc. I was proposed to create a `master` 
>> message for canonical records and use different `causes`, aka 
>> fragments, to be more specific. I decided to follow the same pattern 
>> for accessor methods. Regarding the suggestion of placing the caret 
>> in the throws clause for the corresponding error message, not 
>> possible. We would have to move the check for that error to the parser.
>>
>> New in this iteration:
>> - I have included jdeps related code, I was thinking about putting it 
>> in a separate review bucket but, it is small, it kind of fits with 
>> the rest of the review
>> - a series of supporting libraries for tests under: 
>> test/langtools/lib/combo/tools/javac/combo with also the tests that 
>> needed to be modified due to changes in the lib. These changes were 
>> created by Brian
>> - a minor change done at ToolBox.java
>>
>> Thanks for all the feedback,
>> Vicente
>>
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.03/