Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong CtRole? #1386

Closed
pvojtechovsky opened this issue Jun 9, 2017 · 2 comments · Fixed by #1397
Closed

Wrong CtRole? #1386

pvojtechovsky opened this issue Jun 9, 2017 · 2 comments · Fixed by #1397
Labels

Comments

@pvojtechovsky
Copy link
Collaborator

Is it correct that CtSwitchImpl#cases has role CASE and CtCaseImpl#caseExpression has tole CASE too ? I suggest to use different roles for these different meanings.

CtTypeImpl#typeMembers has role FIELD. I guess it is confusing. The role name should TYPE_MEMBER, because it may contain fields, methods, constructors and inner types.

CtContinueImpl#labelledStatement has role LABEL, it is the same role like CtStatementImpl#label. I guess
CtContinueImpl#labelledStatement should have role TARGET_LABEL.

I also wonder why CtBreakImpl#targetLabel has type String and CtContinueImpl#labelledStatement has type CtStatement. Why that difference?

Is it correct that same roles are using different target types? For example:

  • CtUnaryOperatorImpl#operand has role EXPRESSION and type spoon.reflect.code.CtExpression
  • CtNewClassImpl#anonymousClass has role EXPRESSION and type spoon.reflect.declaration.CtClass<?>

But sometime it is correct, for example:

  • CtExecutableImpl#body has type spoon.reflect.code.CtBlock<?>
  • CtLoopImpl#body has type spoon.reflect.code.CtStatement
    because CtStatement can be converted to CtBlock

I have extracted all 1) field type, 2) field name, 3) role name, 4) declaring type name
into this CSV file fields.txt
You can open it in MS Excel and sort by some column and easily see other inconsistencies too.

@tdurieux
Copy link
Collaborator

tdurieux commented Jun 9, 2017

Sorry for the several bugs.
I made several PR and I mixed a little bit...

Is it correct that CtSwitchImpl#cases has role CASE and CtCaseImpl#caseExpression has tole CASE too ? I suggest to use different roles for these different meanings.

CtCaseImpl#caseExpression I think it's better to use EXPRESSION for this one

CtTypeImpl#typeMembers has role FIELD. I guess it is confusing. The role name should TYPE_MEMBER, because it may contain fields, methods, constructors and inner types.

My bad I forgot to change that one. I think I will but several role on this field (ex: `@MetamodelPropertyField(role = [FIELD, MOTHOD, NESTED_TYPE, CONSTRUCTOR])

CtContinueImpl#labelledStatement has role LABEL, it is the same role like CtStatementImpl#label. I guess
CtContinueImpl#labelledStatement should have role TARGET_LABEL.

We want to remove CtContinueImpl#labelledStatement because it is the statement that is pointed by the label of the continue. We can lookup this value see #1381
I will probably remove the annotation from CtContinueImpl#labelledStatement

Is it correct that same roles are using different target types? For example:
CtUnaryOperatorImpl#operand has role EXPRESSION and type spoon.reflect.code.CtExpression
CtNewClassImpl#anonymousClass has role EXPRESSION and type spoon.reflect.declaration.CtClass<?>

I am not a big fan to put CtNewClassImpl#anonymousClass as an EXPRESSION but I did not want to create an annotation for this case because it is an artificial role. I don't know what to do.

I have extracted all 1) field type, 2) field name, 3) role name, 4) declaring type name
into this CSV file fields.txt
You can open it in MS Excel and sort by some column and easily see other inconsistencies too.

This is useful thanks

@pvojtechovsky
Copy link
Collaborator Author

Sorry for the several bugs.

I understand this meta model stuff as incubation = not finish, bug full, to be changed ..., so I really do not complain! I just wanted to point to the potential problems , so they are found and solved sooner. Thank You for Your effort!

CtTypeImpl#typeMembers
I think I will add several roles on this field
`@metamodelpropertyfield(role = [FIELD, METHOD, NESTED_TYPE, CONSTRUCTOR])

This is conceptual question - How many roles may have a field?
A) each field has exactly one role
B) each field has one or more roles

In some use cases, the one role is correct. In others the more roles are correct. So solution might be:

  • to replace role attribute, by two attributes: logicalRole, phisicalRole. The logicalRole would be 1:N, the phisicalRole would be 1:1
  • to introduce hierarchy into roles: to be able to define that role TYPE_MEMBER is a superType of roles: FIELD, METHOD, NESTED_TYPE, CONSTRUCTOR. May be this hierarchy does not have to be expressed in java code. It might be documented only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants