feature: CtRole#getSuperRole(), CtRole#getSubRole()

[pvojtechovsky]

I need that in new TemplateMatcher to distinguish between derived attributes and real attributes.
E.g. CtRole.CONSTRUCTOR.getSuperRole() == CtRole.TYPE_MEMBER

[surli]

Instead of doing this static loop, can't you make the assignment in a constructor of CtRole?

[pvojtechovsky]

I wanted to do that, but java compiler has problem with that.

private static final CtRole[] EMPTY = new CtRole[0];
private CtRole superRole = EMPTY; //<-- compilation error. 

I wonder why too..,. but it is so.

[surli]

Some hints about why it does not work: https://stackoverflow.com/questions/29174345/illegal-reference-to-static-field-from-initializer

However it will work if you do not use a constant but directly private CtRole[] subRoles = new Ctrole[0];.

[pvojtechovsky]

You are right, this approach is nicer and we do not have to care about memory for these few cases.

I have added also test. It is ready for merge from my point of view.

[pvojtechovsky]

Could you please merge it? Or is there any problem? ;-)

[monperrus]

I have one problem here :-)

The problem is to have a mutable enum, which can be considered as a serious design issue.

Do you need to have variable super relationships? or would it be possible to hard code the required super relationships in an immutable enum eg:

METHOD(super_role = TYPE_MEMBER)

[pvojtechovsky]

I think it is not possible to make it completely immutable, because we cannot refer enum items in their constructor, which are not yet created.
Note that only enum itself can mutate because all is private. Of course the reflection might be used to mutate private values. But that will work also for final values ... https://stackoverflow.com/questions/3301635/change-private-static-final-field-using-java-reflection

So if you want any changes please try to implement them. I have no idea how to do it better.

[surli]

Personally I don't have a problem with the fact that it makes the enum potentially mutable: it's encapsulated inside the enum and everything's private.

However, you're usage here is very limited: it's only relationship between TypeMembers and few associated roles. Unless you already expect that there will be other relationship, else why not hard coded this data directly in getSuperRole() and getSubRole(). I agree it's uglier, but at least it keeps the enum completely immutable.

[pvojtechovsky]

I changed CtRole[] getSubRoles() to List<CtRole> getSubRoles(), which returns immutable List. I also made properties final, so now it is really immutable...

[pvojtechovsky]

@monperrus , is it enough immutable? Or too much ugly? ;-)

I had to fix also ModelRoleHandler, which generated depending on order of roles in CtRole definition. Now it is independent on that order and it is sorted by name.

[monperrus]

It looks good!

TYPE_MEMBER(true) is hard to understand.

I'd propose to remove the constructor with boolean, TYPE_MEMBER(true), the list of subroles, and to compute subroles on the fly in a stateless manner.

[pvojtechovsky]

compute subroles on the fly in a stateless manner.

I do not like such "slowing down loops" if they are not necessary for simpler maintenance. I guess this solution is acceptable now.

[monperrus]

Thanks a lot Pavel.

I do not like such "slowing down loops" if they are not necessary for simpler maintenance

This is where we disagree. I tend to think that statelessness is essentially good for reliability (less bugs) and maintenance (easier to debug and understanding). I'm OK to trade statelessness for performance once there is a clear use case with a target metric (eg < 1 sec).