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

[ROASTER-64] Improve the handling of generics - RFC #33

Closed
wants to merge 3 commits into from

Conversation

sotty
Copy link
Contributor

@sotty sotty commented Mar 9, 2015

Some further formatting fixes may be needed, but need the approach validated first

@forge-bot
Copy link
Contributor

Build 48 is now running using a merge of 4ee164e

@forge-bot
Copy link
Contributor

Build 48 is now running using a merge of 4ee164e

@forge-bot
Copy link
Contributor

Build 48 outcome was FAILURE using a merge of 4ee164e
Summary: Tests failed: 8 (8 new), passed: 738, ignored: 52 Build time: 0:01:16

Build problems:

Failed tests detected

Failed tests

org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeJavaType(boolean BigInterface.verbose): java.lang.NullPointerException: null
    at org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeJavaType(PropertiesTest.java:433)

org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeClass(boolean BigInterface.verbose): java.lang.NullPointerException: null
    at org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeClass(PropertiesTest.java:403)

org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeString(boolean BigInterface.verbose): java.lang.NullPointerException: null
    at org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeString(PropertiesTest.java:418)

org.jboss.forge.test.roaster.model.MethodSignatureTest.testMethodWithInnerClassParameter: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
    at java.util.ArrayList.rangeCheck(ArrayList.java:653)
    at java.util.ArrayList.get(ArrayList.java:429)

org.jboss.forge.test.roaster.model.MethodSignatureTest.testMethodWithPrimitiveParameters: java.lang.AssertionError: expected:<3> but was:<1>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:743)

org.jboss.forge.test.roaster.model.JavaClassMethodTest.testAddParameter: java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)

org.jboss.forge.test.roaster.model.JavaClassMethodTest.testAddParameterJavaType: java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)

org.jboss.forge.test.roaster.model.JavaClassMethodTest.testAddParameterStringType: java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)

@forge-bot
Copy link
Contributor

Build 48 outcome was FAILURE using a merge of 4ee164e
Summary: Tests failed: 8 (8 new), passed: 738, ignored: 52 Build time: 0:01:32

Build problems:

Failed tests detected

Failed tests

org.jboss.forge.test.roaster.model.JavaClassMethodTest.testAddParameter: java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)

org.jboss.forge.test.roaster.model.JavaClassMethodTest.testAddParameterJavaType: java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)

org.jboss.forge.test.roaster.model.JavaClassMethodTest.testAddParameterStringType: java.lang.AssertionError: null
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)

org.jboss.forge.test.roaster.model.MethodSignatureTest.testMethodWithInnerClassParameter: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
    at java.util.ArrayList.rangeCheck(ArrayList.java:638)
    at java.util.ArrayList.get(ArrayList.java:414)

org.jboss.forge.test.roaster.model.MethodSignatureTest.testMethodWithPrimitiveParameters: java.lang.AssertionError: expected:<3> but was:<1>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:743)

org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeJavaType(boolean BigInterface.verbose): java.lang.NullPointerException: null
    at org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeJavaType(PropertiesTest.java:433)

org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeClass(boolean BigInterface.verbose): java.lang.NullPointerException: null
    at org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeClass(PropertiesTest.java:403)

org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeString(boolean BigInterface.verbose): java.lang.NullPointerException: null
    at org.jboss.forge.test.roaster.model.PropertiesTest.testSetTypeString(PropertiesTest.java:418)


private static final Pattern SIMPLE_ARRAY_PATTERN = Pattern
.compile("^((.)+)(\\[\\])+$");
private static final Pattern IDENTIFIER_PATTERN = Pattern
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These regexps are the critical issue to be discussed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generics, due to the balanced angular brackets, require a context-free grammar, e.g.
T -> ID Gen
Gen -> '<' T '>'
And context-free grammars are not regular, which means that they can't be recognized by a regular expression

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent. I actually had no idea you could call Character methods through the POSIX \p. Looks like I need to add something to my regular expression tutorials!

@sotty
Copy link
Contributor Author

sotty commented Mar 9, 2015

I still need to complete the work and fix a few failing tests, but I'd like some feedback on the regexp issue

String stub = "public class Stub { public void method( " + Types.toSimpleName(Types.fixArray(type, false)) + " "
+ name + " ) {} }";
String resolvedType = TypeImpl.ensureImports(getOrigin(), new TypeImpl<O>(getOrigin(),null,type));
String stub = "public class Stub { public void method( " + resolvedType + name + " ) {} }";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests fail just because a + " " + was lost between resolvedType and name :)
If we agree on the rest, I'll amend the PR

@forge-bot
Copy link
Contributor

Build 49 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 49 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:01:15

@@ -722,6 +723,11 @@ public boolean isAnnotation()
}

@Override
public String ensureImports(Type<?> type) {
throw new UnsupportedOperationException("Imports cannot be added at the package-info level");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, you can import package level annotations among other things.. I'll fix it

@forge-bot
Copy link
Contributor

Build 50 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 50 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 50 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:00:28

@forge-bot
Copy link
Contributor

Build 50 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:00:40

@forge-bot
Copy link
Contributor

Build 51 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 51 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 51 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:00:32

@forge-bot
Copy link
Contributor

Build 51 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:01:20

@forge-bot
Copy link
Contributor

Build 52 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 52 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 52 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:00:29

@forge-bot
Copy link
Contributor

Build 52 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 746, ignored: 52 Build time: 0:00:35

@forge-bot
Copy link
Contributor

Build 53 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 53 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 53 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 750, ignored: 52 Build time: 0:00:25

@forge-bot
Copy link
Contributor

Build 53 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 750, ignored: 52 Build time: 0:00:26

Conflicts:
	impl/src/main/java/org/jboss/forge/roaster/model/impl/MethodImpl.java
@forge-bot
Copy link
Contributor

Build 54 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 54 is now running using a merge of abea144

@forge-bot
Copy link
Contributor

Build 54 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 751, ignored: 52 Build time: 0:00:27

@forge-bot
Copy link
Contributor

Build 54 outcome was SUCCESS using a merge of abea144
Summary: Tests passed: 751, ignored: 52 Build time: 0:00:28

@forge-bot
Copy link
Contributor

Build 55 is now running using a merge of 433f8e8

@forge-bot
Copy link
Contributor

Build 55 is now running using a merge of 433f8e8

@forge-bot
Copy link
Contributor

Build 55 outcome was SUCCESS using a merge of 433f8e8
Summary: Tests passed: 752, ignored: 52 Build time: 0:00:24

@forge-bot
Copy link
Contributor

Build 55 outcome was SUCCESS using a merge of 433f8e8
Summary: Tests passed: 752, ignored: 52 Build time: 0:00:25

* @return The name (simple or fully qualified) that should be used to reference the imported type in the code
* @seeAlso org.jboss.forge.roaster.model.Type
*/
public Import addImport(Type<?> type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good

@forge-bot
Copy link
Contributor

Build 56 is now running using a merge of 80de718

@forge-bot
Copy link
Contributor

Build 56 is now running using a merge of 80de718

@forge-bot
Copy link
Contributor

Build 56 outcome was SUCCESS using a merge of 80de718
Summary: Tests passed: 752, ignored: 52 Build time: 0:00:23

@forge-bot
Copy link
Contributor

Build 56 outcome was SUCCESS using a merge of 80de718
Summary: Tests passed: 752, ignored: 52 Build time: 0:00:26

@gastaldi
Copy link
Member

gastaldi commented May 4, 2015

Merged in http://github.com/forge/roaster/commit/541472e80. Thanks!

@gastaldi gastaldi closed this May 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants