Bug 76190 - DCR AST: EnumDeclaration: Separate field and body statement lists
Summary: DCR AST: EnumDeclaration: Separate field and body statement lists
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.1 M3   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-10-13 13:50 EDT by Martin Aeschlimann CLA
Modified: 2004-11-02 11:20 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Aeschlimann CLA 2004-10-13 13:50:48 EDT
20041013

The body of an EnumDeclaration is defined as 
 *         <b>{</b>
 *         [ EnumConstantDeclaration { <b>,</b> EnumConstantDeclaration } ] [
<b>,</b> ]
 *         [ <b>;</b> { ClassBodyDeclaration | <b>;</b> } ]
 *         <b>}</b>

In the API of 'EnumDeclaration' the two lists (constants and classbody
declaration) are modelled in one list (body declarations)

Wouldn't it be better to have this is two lists? I'm currently working on the
AST rewriter and this gets quite tricky: The separator between the list nodes
depends on the node type (separators are ',', ';' and new lines)
Replacing a node can result in changing the separator before and after the node.
This is of course all doable, but represents a special case, no other list of
nodes is as complicated.
Any chance that the lists can be separated? I also think the merging of the
lists is also conceptionally a hack.
Comment 1 Jim des Rivieres CLA 2004-10-13 15:02:02 EDT
I agree that the situation is not quite perfect. Let me explain how it came to 
be the way it is. EnumDeclaration is declared as a subclass of 
AbstractTypeDeclaration; this was necessary because there needed to be 
something like this to encompass all forms for type declarations. It's a good 
fit for AbstractTypeDeclaration to declare bodyDeclarations() because these 
are what normal types and annotation types have. This is, as you point out, a 
bit of a stretch for enum declarations, which really have a pair of lists. The 
first list is the enum constants (EnumConstantDeclaration), and the second 
list is the (optional) class body declarations. Rather than introducing a 
second list, I combined them into one. EnumConstantDeclarations are typically 
very simple (just an identifier), but in complex forms are almost 
indistinguishable from method declarations. This is why it did not feel like 
too much of a stretch. The convienience method getEnumConstants() makes it 
easy for clients to get at the constants. And I thought it would be easy for 
the DOM/AST implementation to put the ";" in the right place when serializing.

The obvious alternative is to add a second list property:
    public List<EnumConstantDeclaration> enumConstants();
and spec bodyDeclarations() to exclude the enum constants.

Do you think that change would make it any easier or harder for clients?

Comment 2 Martin Aeschlimann CLA 2004-10-14 04:44:49 EDT
When I first looked at the EnumDeclaration I studied the grammar and then 
wanted to find the enum constant list in the API. I first tought it was 
forgotten until I understood that the two lists have been merged.
So, yes, I think separation would make the API clearer. Manipulations and 
access on the node would benefit, having a strong typing of the enum constant 
list, no worries about a illegal order of element (you can't have a normal 
body declaration between constants). It's more explicit and this is more the 
style of the rest of the ASTNode API's.

I guess the conterargument is that the new list addition would break clients 
already using the EnumDeclaration. The damage would still be small, the nodes 
haven't been parsed until recently, so I would vote for it.
Comment 3 Philipe Mulet CLA 2004-10-14 09:45:07 EDT
FYI - I am currently changing the compiler internals to separate the
enumConstants from their body declarations. In essence, an enum constant is
homogeneous to a field, and it may additionally carry an anonymous type (in the
same way as a qualified allocation expression).
Comment 4 Jim des Rivieres CLA 2004-10-14 15:29:00 EDT
Ok. Added a second list property to EnumDeclaration:
    public List<EnumConstantDeclaration> enumConstants();
Deprecated method on EnumDeclaration (to be deleted after 3.1M3):
    public EnumConstantDeclaration[] getEnumConstants();
Changed spec for EnumDeclaration:
    bodyDeclarations() does not include enum constant declarations

Olivier, please fix AST converter.
Comment 5 Olivier Thomann CLA 2004-10-14 15:44:11 EDT
I am investigating the changes.
Comment 6 Olivier Thomann CLA 2004-10-14 17:35:55 EDT
Fixed and released in HEAD.
Tests have been updated to use the new API.
Comment 7 Martin Aeschlimann CLA 2004-10-16 04:52:31 EDT
ast rewrite updated, tests added > 20041015
Comment 8 David Audel CLA 2004-11-02 11:20:38 EST
Verified for 3.1M3 with build I20041101