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

[Maurizio Cimadamore]

No worries!


Thanks
Maurizio


On 05/02/18 17:04, Srinivas Dama wrote:
> Sorry, no more reviews are required.
>
> Regards,
> Srinivas
> ----- Original Message -----
> From: srinivas.dama at oracle.com
> To: compiler-dev at openjdk.java.net
> Sent: Monday, February 5, 2018 3:09:44 PM GMT +05:30 Chennai, Kolkata, 
> Mumbai, New Delhi
> Subject: Re: RFR: 8152616: com.sun.tools.javac.tree.Pretty generates 
> nested comments for enum
>
> 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/
>     <http://cr.openjdk.java.net/%7Esdama/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/
>         <http://cr.openjdk.java.net/%7Esdama/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/3825fe58/attachment-0001.html>