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

COLLECTIONS-701 SetUniqueList.add() crashes due to infinite recursion… #57

Closed
wants to merge 3 commits into from

Conversation

@drajakumar
Copy link
Contributor

commented Nov 8, 2018

… when it receives itself

@garydgregory

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

Hi @drajakumar ,

I'm not sure this patch makes sense. Take a look at org.apache.commons.collections4.list.Collections701Test: For ArrayList and HashSet, adding a collection to itself is fine.

In this patch, the argument is not only silently ignored, but the behavior is not even documented. Whatever we do, we really need to document anything that deviates from the standard JRE List contract.

IMO, the fix should be so that a SetUniqueList behaves like a ArrayList and HashSet, it just works.

@drajakumar

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

closing the pr as the fix is not as expected

@drajakumar drajakumar closed this Nov 9, 2018

@garydgregory

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

@drajakumar, You did not have to close the PR, I was hoping you would provide a more complete solution ;-)

@drajakumar

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

sure @garydgregory i am analyzing for a fix as per the expectation, will update you.

@drajakumar drajakumar reopened this Nov 11, 2018

@drajakumar

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2018

@garydgregory can you kindly check the new fix, thank you!

1 similar comment
@drajakumar

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@garydgregory can you kindly check the new fix, thank you!

@asfgit asfgit closed this in 1979a6e Nov 23, 2018

asfgit pushed a commit that referenced this pull request Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.