RFR: 8152616: com.sun.tools.javac.tree.Pretty generates nested comments for enum

[Srinivas Dama]

Hi, 

May I have second review for this to push. 

Regards, 
Srinivas 
----- Original Message ----- 
From: maurizio.cimadamore at oracle.com 
To: srinivas.dama at oracle.com, compiler-dev at openjdk.java.net 
Sent: Friday, February 2, 2018 2:37:00 PM GMT +05:30 Chennai, Kolkata, Mumbai, New Delhi 
Subject: Re: RFR: 8152616: com.sun.tools.javac.tree.Pretty generates nested comments for enum 




Looks good, thanks! 

Maurizio 


On 02/02/18 05:41, Srinivas Dama wrote: 





Hi Maurizio, 



Thank you for the comments. 

Please review the revised webrev at http://cr.openjdk.java.net/~sdama/8152616/webrev.01/ . 



> * why the logic for printing the enum constant constructor type arguments? It is not possible to specify explicit constructor arguments on enum constants. (Funnily, javac attempts to parse them, but the attempt fails because the parser mode is wrong, so an >error is generated). The spec has no place for them in the grammar: 

Yes- no need to print enum constant constructor type arguments, earlier I have taken this from existing “visitNewClass” method which is doing so but missed to remove that. 

> I believe your end of block comment is misplaced, and it skips enum constructor arguments - that is, I see that '*/' is put AFTER the constructor arguments and before the '{' if any. Now, that doesn't affect your test, because the constants in the test don't >have constructor arguments, but I believe there's bug lurking in here? 

Yes – there is a bug hidden here.Fixed and modified the test case as well. 

Regards, 

Srinivas 





From: Maurizio Cimadamore 
Sent: Thursday, January 25, 2018 4:48 PM 
To: Srinivas Dama <srinivas.dama at oracle.com> ; compiler-dev at openjdk.java.net 
Subject: Re: RFR: 8152616: com.sun.tools.javac.tree.Pretty generates nested comments for enum 



Hi Srinivas, 
two comments: 

* why the logic for printing the enum constant constructor type arguments? It is not possible to specify explicit constructor arguments on enum constants. (Funnily, javac attempts to parse them, but the attempt fails because the parser mode is wrong, so an error is generated). The spec has no place for them in the grammar: 

https://docs.oracle.com/javase/specs/jls/se9/html/jls-8.html#jls-8.9.1 

* I believe your end of block comment is misplaced, and it skips enum constructor arguments - that is, I see that '*/' is put AFTER the constructor arguments and before the '{' if any. Now, that doesn't affect your test, because the constants in the test don't have constructor arguments, but I believe there's bug lurking in here? 

Cheers 
Maurizio 




On 25/01/18 08:19, Srinivas Dama wrote: 



Hi, 



Please review http://cr.openjdk.java.net/~sdama/8152616/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8152616 



Regards, 

Srinivas 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180205/54f9a2d9/attachment.html>