RFR: JEP 359-Records: compiler code

Wed Oct 30 18:07:53 UTC 2019

Hi Srikanth,

Thanks for the review, comments inlined below.

On 10/28/19 5:10 AM, Srikanth wrote:
> Hi Vicente,
>
> I plan to make one more separate pass over annotation handling later 
> this week, in the
> mean time, here are several comments.
>
> I omitted anything already called out by Maurizio or Jan.
>
> HTH,
> Thanks
> Srikanth
>
>
> (1) Flags.java: Is there a reason for choosing the flags for RECORD to 
> be 1L<<61, leaving 1<<59 and 1<<60 as holes ??

no reason, I have covered the gap

>
> (2) javadoc for flag RECORD inconsistent with the // comment (one 
> mentions methods the other does not)

true, I have fixed that

>
> (3) MemberRecordClassFlags LocalRecordFlags - naming inconsistent; 
> former should be MemberRecordFlags ?
fixed
>
> (4) com.sun.tools.javac.parser.JavacParser#isRestrictedRecordTypeName 
> appears unused
removed
>
> (5) (Ignorable) Field `names' is changed from private to protected 
> because it needs to be referenced in ReplParser I guess. This could 
> have been worked around by reaching to token.name().table.names.record 
> instead.

I have fixed that too

>
> (6) Javac allows a top level record to be static - is this intentional 
> ? Top levels classes may not be.
> This code compiles:
> static public record X(int x, int y) {
>
> }

correct, this was already fixed in the last iteration

>
> (7) Local records cannot be explicitly tagged final. (Likewise 
> annotations will be rejected)
>
> public class X {
>     public static void main(String [] args) {
>         final record X(int x, int y) {  // <<-- does not compile
>         }
>     }
> }

fixed

>
>
> (8) if (isRecordToken() &&
>             (peekToken(TokenKind.IDENTIFIER, TokenKind.LPAREN) ||
>              peekToken(TokenKind.IDENTIFIER, TokenKind.LT))) {
>             JCModifiers mods = modifiersOpt();
>             dc = token.comment(CommentStyle.JAVADOC);
>             return List.of(recordDeclaration(mods, dc));
>         }
>
> The call to modifiersOpt - is this misplaced ? We have already seen 
> `record' and the peek is signalling an IDENTIFIER ??
>
> (The default is moved out of the switch - it took me a while to 
> recognize that the semantics are unaffected. So this appears fine)

actually I think that you are right, the call to modifiersOpt seems to 
be unnecessary at that point. I have removed it
>
> (9) I was surprised to see the emission of Errors.RecordCantBeAbstract in
> com.sun.tools.javac.parser.JavacParser#recordDeclaration.
> I would have expected this check inside 
> com.sun.tools.javac.comp.Check#checkFlags
> See that this latter place is where abstract enums are complained 
> against.

right it was actually been also checked in Check::checkFlags, I have 
removed the check in JavacParser
>
> (10) List<JCTree> defs = List.nil(); inside 
> JavacParser#recordDeclaration is a redundant initialization.
fixed
>
> (11) Likewise I was surprised to see 
> Errors.RecordCantDeclareDuplicateFields being emitted
> in com.sun.tools.javac.parser.JavacParser#recordDeclaration.
> I would have expected this to be emitted in 
> com.sun.tools.javac.comp.Check#checkUnique

right, there are two opposed forces here. The check is in JavacParser 
because record components exists until JavacParser. From that point on, 
they are not record components anymore but fields and parameters to a 
constructor. So if they are repeated then we would be talking to the 
user about fields and method parameters that he didn't declare he just 
declared record components. That's why the code is there. I can move it 
but then the user will probably have spurious error messages.

>
> (12) Compact constructor trees are modified in in 
> com.sun.tools.javac.parser.JavacParser#recordDeclaration to become 
> elaborated tree - I thought this practice of modifying parse trees in 
> the early stages is frowned upon ?? (Other parse tree transformations 
> also happen here)

right but no other feature allows to declare a parameter-less method. If 
not expanded here the compact constructor would have to be expanded in 
TypeEnter, which probably is not very nice either, so I didn't see a 
perfect solution

>
> (13) com.sun.tools.javac.parser.JavacParser#formalParameter() is 
> unused ??

yep removed, I created another flavor that must be used by all clients

>
> (14) Check.java: Seems import java.util.stream.Collectors; is unused

I see one use of it at:

boolean merge(ListBuffer<JCLiteral> litBuf, ListBuffer<JCExpression> opStack) {}

>
> (15) Symbol.java: several unused imports seem to have crept in.
> import com.sun.tools.javac.util.Name;
> import com.sun.tools.javac.code.MissingInfoHandler;
> import com.sun.tools.javac.code.Symbol;
> import static 
> com.sun.tools.javac.code.Symbol.OperatorSymbol.AccessCode.FIRSTASGOP;
> import com.sun.tools.javac.code.Type;

removed

>
> (16) The various isXXX methods differ in how they access the 
> flags_field. Some use
> flags() while others use flags_field. I think it is better to use 
> flags() as it will
> trigger completion (as it should ??) In any case the difference seems 
> gratuitous

I would prefer not to trigger completion, I have moved all of them to 
use the field
>
> (17) Does com.sun.tools.javac.code.Symbol.MethodSymbol#isDynamic serve 
> a purpose - seems
> to be identical to super implementation ??

no, removed, it probably ended there after a merge and I oversaw it

>
> (18) Attr: Errors.MethodMustBePublic emission code could use the new 
> Symbol.isPublic()
> (100 lines below we do use the new method)
done
>
> (19) Attr.isUnchecked() the new methods are mirror images of the 
> Check.isUnchecked methods ?
> Is there a reason for the duplication ??
those were removed already
>
> (20) Javadoc of 
> com.sun.tools.javac.comp.Attr#checkFirstConstructorStat does not
> document all parameters
done
>
> (21) I didn't quite understand what we do this in MemberEnter for:
>     else if (v.owner.kind == MTH || (v.flags_field & (Flags.PRIVATE | 
> Flags.FINAL | Flags.MANDATED | Flags.RECORD)) != 0) {
>             enclScope.enter(v);
>         }
>
I have added a comment, that means that we are dealing with a field that 
was obtained from a record component. It should be entered

Thanks for all the comments,

Vicente

PS, I will be generating another webrev that will also include last 
comments from Maurizio

>
> On 21/10/19 6:01 PM, Vicente Romero wrote:
>> Hi,
>>
>> Please review the compiler code for JEP 359 (Records) [1]
>>
>> Thanks in advance for the feedback,
>> Vicente
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/compiler/webrev.00/
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191030/0bf92f5f/attachment.html>