RFR: JEP 359-Records: compiler code
Srikanth
srikanth.adayapalam at oracle.com
Mon Oct 28 09:10:35 UTC 2019
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/
More information about the compiler-dev
mailing list