-
Notifications
You must be signed in to change notification settings - Fork 38
add fix to identityhashmap, add one test class for it #212
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| The MIT License (MIT) | ||
| Copyright (c) 2015 Alex Gyori | ||
| Copyright (c) 2022 Kaiyao Ke | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add your name here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The checkstyle enforces fixed header so I couldn't add my name in it. I added my name in comment above the test case instead, is this ok?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine as you did it, although we/you could have changed the Checkstyle config. |
||
| Copyright (c) 2015 Owolabi Legunsen | ||
| Copyright (c) 2015 Darko Marinov | ||
| Copyright (c) 2015 August Shi | ||
|
|
||
|
|
||
| Permission is hereby granted, free of charge, to any person obtaining | ||
| a copy of this software and associated documentation files (the | ||
| "Software"), to deal in the Software without restriction, including | ||
| without limitation the rights to use, copy, modify, merge, publish, | ||
| distribute, sublicense, and/or sell copies of the Software, and to | ||
| permit persons to whom the Software is furnished to do so, subject to | ||
| the following conditions: | ||
|
|
||
| The above copyright notice and this permission notice shall be | ||
| included in all copies or substantial portions of the Software. | ||
|
|
||
| THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, | ||
| EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | ||
| MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND | ||
| NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE | ||
| LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION | ||
| OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION | ||
| WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
| */ | ||
|
|
||
| package edu.illinois.nondex.core; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.IdentityHashMap; | ||
| import java.util.Iterator; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class IdentityHashMapTest { | ||
|
|
||
| // Author: Chih-Fu Lai (2025) | ||
| // Added test coverage for IdentityHashMap.removeAll iterator behavior | ||
| @Test | ||
| public void testRemoveAllArrayIndexOutOfBounds() { | ||
| for (int i = 0; i < 999; i++) { | ||
| IdentityHashMap<Object, Object> map = new IdentityHashMap<>(4); | ||
|
|
||
| Object k1 = new Object(); | ||
| Object k2 = new Object(); | ||
| Object k3 = new Object(); | ||
|
|
||
| map.put(k1, 1); | ||
| map.put(k2, 2); | ||
| map.put(k3, 3); | ||
|
|
||
| map.keySet().removeAll(Collections.singleton(k2)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some assertion to check (e.g., that the size of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I am simply checking that this doesn't throw an exception. I consider it redundant to add additional assertions because most map behaviors are already checked by tests in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, fine to not have an assertion. |
||
| } | ||
| } | ||
| } | ||
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.
How did you write this code?
Uh oh!
There was an error while loading. Please reload this page.
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.
I simply follow each line in the original
removemethod and convert each line into asm visitor by referring to existing NonDex code likeaddHasNextmethods. The one part I had to find other reference and ask LLM for help is the Complex condition part, which requires some complex jump instructions. But overall the syntax are taken from other existing part in the file.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.
Ah, sorry we didn't tell you about ASMifier https://asm.ow2.io/faq.html#Q10
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.
@EdwinIngJ did tell me about the tool, I used it to verify my instructions.
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 you tested your fix across multiple Java versions (for example, Java 8)? In some cases, the implementation can differ between versions.
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.
Please double check
java -version. If you're running on your VM, please email me the details so I can try out myself.Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry I was accidentally using the Oracle JDK 25 instead of OpenJDK 25. Now the build and test works fine. @EdwinIngJ Could you tell me more about the failure with Java 11? I tested in all above Java version including Java 11 and it worked fine.
My Java 11 JDK:
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.
I get the error running the integration tests:
Here is my Java version:
Uh oh!
There was an error while loading. Please reload this page.
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.
I still couldn't reproduce the error myself. Running the integration tests with
mvn clean installand runningmvn clean verify -pl nondex-maven-pluginafterwards all passed for me. TheMapTestin integration test should works exactly the same as the test innondex-testmodule right? Is there a clean way to run the test possibly with different seeds likenondexRuns?My integration tests result:
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.
Hmm, I also ran the same commands. I'm not aware of a clean way of specifying the seed in the test. But Github action was recently added to
master. Try rebasing on top ofmasterand let the Github action run and maybe that will provide more insight.