-
Notifications
You must be signed in to change notification settings - Fork 0
Reduce code duplication #1
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
Reduce code duplication #1
Conversation
| stringToConstant = new HashMap<>(); | ||
| constantToName = new EnumMap<>(classOfT); | ||
| // Don't use `EnumMap` here; that can break when using ProGuard or R8 | ||
| constantToName = new HashMap<>(); |
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.
Have reverted this to a HashMap because the EnumMap was causing issues for the test-shrinker integration tests; the JDK was unable to create the EnumMap for the enum
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.
What issue does EnumMap have? I'm not sure because we have the classOfT here
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.
Run mvn clean verify --projects test-shrinker --also-make, it will fail with the following error when using EnumMap:
...
Caused by: java.lang.RuntimeException: Test failed: Write: Enum
at com.example.ai.a(Unknown Source)
at com.example.Main.runTests(Unknown Source)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
... 39 more
Caused by: java.lang.NullPointerException: Cannot read the array length because "this.keyUniverse" is null
at java.base/java.util.EnumMap.<init>(EnumMap.java:139)
at com.a.a.b.a.i.<init>(Unknown Source)
at com.a.a.b.a.i.<init>(Unknown Source)
...I think the issue is that ProGuard and R8 shrink the enum class so much that the JDK cannot obtain the enum constants. This is also why EnumTypeAdapter uses class#getDeclaredFields() and isEnumConstant() instead of simply Class#getEnumConstants().
82e935a
into
MukjepScarlet:gson_default
Resolves some of the issues with google#2864, mainly:
GsonBuilderHelperis not creating the list of factories anymoreSee also my review comments below for additional explanations for the changes.
Feel free to adjust the changes here in case you don't want to directly integrate them as is into your PR.
Generally I really like your approach though!