-
Notifications
You must be signed in to change notification settings - Fork 111
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 the issue #103 by create two new FastCloners #109
Conversation
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.
That is wrong, you must also clone the elements of the lists.
ok, I change the code to use cloner.deepClone |
@tweimer is this pr ok now? |
See my comment in the review. A test for both cloners with two elements would be useful,for the sake of completeness. |
# Conflicts: # src/test/java/com/rits/tests/cloning/TestCloner.java
the test cases are now complete |
Like good to me. @kostaskougios has to merge it |
👍 |
Assuming you feel this is stable at this stage, any chance of cutting another release @kostaskougios and @tweimer ? |
Any chance of 1.11.0 making it to maven central so i can get this fix? |
@atribemc Seems unlikely based on #105 (comment) I did start to consider maintaining/releasing a fork at https://github.com/chadlwilson/cloning but haven't pushed it anywhere yet/set up automation, as for my use case we had our own fast cloners for the issue in this PR and we needed I'm not sure if any other possible fork candidates have emerged... |
To fix issue #103
In jdk 5,
cloner.deepClone(Set.of(1)).size()
will be 2 becauseEMPTY
( it isnull
in jdk14 ) will be changed with clone. So it has to create new FastCloner.