-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
fix: compare package names for equality between CtType #5556
Conversation
CtExpression<?> evaluated = ctBinaryOperator.partiallyEvaluate(); | ||
assertNotNull( | ||
evaluated.getType(), | ||
String.format("type of '%s' is null after evaluation", ctBinaryOperator) | ||
); | ||
assertEquals(currentType, evaluated.getType().getTypeDeclaration()); | ||
assertEquals(ctBinaryOperator.getType(), evaluated.getType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should only check for the type of the binary operator and the evaluated result. There is no need to check the equality between type declarations.
@@ -91,6 +91,7 @@ public void testSpoonifier() throws ClassNotFoundException, MalformedURLExceptio | |||
testSpoonifierWith("src/test/java/spoon/test/spoonifier/testclasses/ArrayRealVector.java", i++); | |||
testSpoonifierWith("src/test/java/spoon/test/prettyprinter/testclasses/FooCasper.java", i++); | |||
testSpoonifierWith("src/test/java/spoon/test/prettyprinter/testclasses/Rule.java", i++); | |||
testSpoonifierWith("src/test/resources/UnnamedPackage.java", i++); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this test just to ensure that classes with unnamed packages
can also be spoonified.
This comment was marked as resolved.
This comment was marked as resolved.
This test is failing because it clones a |
07550ab is a bad solution to the problem above. But it passes the test if I explicitly set the package reference after clone. I will look for a better solution. |
I think so far the parent relationships are not cloned, as that can lead to all kinds of trouble. If you set the parent, it might also attach itself in other ways and vice versa. Changing the parent or setting children potentially modifies quite a bit of the model. I am not quite sure you always want to compare the package name as well. If you do that, there is no real way to do structural comparisons anymore, it is a lot easier to check the package name as well. |
Another plausible solution I thought was to change the definition of |
The solution is discussed here. |
Fixes #5536
The types are unequal because they are in different packages, which affects their fully qualified names. Since
scanCtNamedElement
compares based uponsimpleName
, the comparison would pass because it would not account for package names in names of types.I am proposing changes that would compare types based on qualified names and would take any implicit packages into account. Another thing that I noticed was
getValueByRole(CtRole.NAME)
for a class returns its simple name. So, it is semantically wrong to set the role unequal based on comparing qualified names.