RFR: JEP 359-Records: compiler code

[Srikanth]

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

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

(3) MemberRecordClassFlags LocalRecordFlags - naming inconsistent; 
former should be MemberRecordFlags ?

(4) com.sun.tools.javac.parser.JavacParser#isRestrictedRecordTypeName 
appears unused

(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.

(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) {

}

(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
         }
     }
}

(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)

(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.

(10) List<JCTree> defs = List.nil(); inside 
JavacParser#recordDeclaration is a redundant initialization.

(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

(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)

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

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

(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;

(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

(17) Does com.sun.tools.javac.code.Symbol.MethodSymbol#isDynamic serve a 
purpose - seems
to be identical to super implementation ??

(18) Attr: Errors.MethodMustBePublic emission code could use the new 
Symbol.isPublic()
(100 lines below we do use the new method)

(19) Attr.isUnchecked() the new methods are mirror images of the 
Check.isUnchecked methods ?
Is there a reason for the duplication ??

(20) Javadoc of com.sun.tools.javac.comp.Attr#checkFirstConstructorStat 
does not
document all parameters

(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);
         }



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/