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

retrolambda support & cleaner type detection for lambda's #28

Merged
merged 2 commits into from
Sep 30, 2016

Conversation

csoroiu
Copy link
Contributor

@csoroiu csoroiu commented Sep 29, 2016

Added support for retrolambda library:

  • The lambda's created by this library add a field named instance inside the generated classes). I will try to figure out how I can create a test that imitates this behavior.
  • I removed the version restrictions for the lambda support.

Also I did a refactor and made the code work without the TypeDescriptor class. Not sure how to test android compatibility, but I expect it to work.

@codecov-io
Copy link

codecov-io commented Sep 29, 2016

Current coverage is 80.28% (diff: 76.66%)

Merging #28 into master will increase coverage by 2.12%

@@             master        #28   diff @@
==========================================
  Files             2          1     -1   
  Lines           293        213    -80   
  Methods           0          0          
  Messages          0          0          
  Branches         89         73    -16   
==========================================
- Hits            229        171    -58   
+ Misses           31         14    -17   
+ Partials         33         28     -5   

Powered by Codecov. Last update c97c12b...b7a7fa6

@csoroiu csoroiu closed this Sep 29, 2016
@csoroiu
Copy link
Contributor Author

csoroiu commented Sep 29, 2016

Re-opening. Apparently there was an issue when I used retro lambda and jacoco in the same project, but is issue has nothing to do with this change.

Thanks

@csoroiu csoroiu reopened this Sep 29, 2016
@csoroiu csoroiu mentioned this pull request Sep 29, 2016
Copy link
Owner

@jhalterman jhalterman left a comment

Choose a reason for hiding this comment

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

I really like this change a lot. A few comments though - have a look at those.

@@ -250,7 +239,7 @@ public static Type resolveGenericType(Class<?> type, Type subType) {
Map<TypeVariable<?>, Type> map = ref != null ? ref.get() : null;

if (map == null) {
map = new HashMap<TypeVariable<?>, Type>();
map = new HashMap<>();
Copy link
Owner

Choose a reason for hiding this comment

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

TypeTools compilation is currently targeted at 1.7, so I don't think we can use the diamond operator.

Copy link
Contributor Author

@csoroiu csoroiu Sep 30, 2016

Choose a reason for hiding this comment

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

I realized that later and the next commit b7a7fa6 fixes the diamond issue.

@@ -282,7 +271,7 @@ public static Type resolveGenericType(Class<?> type, Type subType) {
}

if (CACHE_ENABLED)
typeVariableCache.put(targetType, new WeakReference<Map<TypeVariable<?>, Type>>(map));
typeVariableCache.put(targetType, new WeakReference<>(map));
Copy link
Owner

Choose a reason for hiding this comment

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

Same here regarding the diamond operator.

@@ -296,7 +285,7 @@ private static void populateLambdaArgs(Class<?> functionalInterface, final Class
if (GET_CONSTANT_POOL != null) {
// Find SAM
for (Method m : functionalInterface.getMethods()) {
if (!m.isDefault() && !Modifier.isStatic(m.getModifiers()) && !m.isBridge()) {
if (!isDefaultMethod(m) && !Modifier.isStatic(m.getModifiers()) && !m.isBridge()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure why this change was necessary?

Copy link
Contributor Author

@csoroiu csoroiu Sep 30, 2016

Choose a reason for hiding this comment

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

I added that check because I removed the version check at the class initialization and it would fail with NoSuchMethodError when ran with java 6 or 7.

@jhalterman jhalterman merged commit 3527d75 into jhalterman:master Sep 30, 2016
@jhalterman
Copy link
Owner

Looks good - thanks for this!

@jhalterman
Copy link
Owner

0.4.8 is released, with this change.

@csoroiu
Copy link
Contributor Author

csoroiu commented Sep 30, 2016

Thanks a lot.

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.

3 participants