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.

Sign up for GitHub

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

8275509: ModuleDescriptor.hashCode isn't reproducible across builds #6078

Closed
wants to merge 17 commits into from
+92 −4
Closed

8275509: ModuleDescriptor.hashCode isn't reproducible across builds #6078

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
73a56ec
8275509: ModuleDescriptor.hashCode isn't reproducible across builds
jaikiran Oct 21, 2021
f583dce
minor test comment fix
jaikiran Oct 22, 2021
1396059
no need to check for null values
jaikiran Oct 22, 2021
c4ad806
drop final
jaikiran Oct 22, 2021
89669ea
drop another final to make it consistent with rest of the code
jaikiran Oct 22, 2021
006255d
add newline at end of test class
jaikiran Oct 25, 2021
60deab1
remove commented out TODO in test
jaikiran Oct 29, 2021
43bd6d8
review suggestion - change requirements to requires
jaikiran Nov 1, 2021
9db9ad4
review suggestion - rename the fromBootLayer method to something that…
jaikiran Nov 1, 2021
8442512
review suggestion - remove use of final in test case
jaikiran Nov 1, 2021
8cfcef1
review suggestion - rename assertTestPrerequisite to something more c…
jaikiran Nov 1, 2021
065c311
restructure the test case to move out the actual testing logic into t…
jaikiran Nov 1, 2021
e8f19d8
remove unintentional change in test case
jaikiran Nov 1, 2021
dee4197
better method name in test class
jaikiran Nov 1, 2021
3552edf
Test case changes:
jaikiran Nov 2, 2021
9dd2774
simplify the test based on review inputs
jaikiran Nov 3, 2021
a0c4f84
use Enum#name() for hashCode() generation instead of Enum#ordinal()
jaikiran Nov 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/java.base/share/classes/java/lang/module/ModuleDescriptor.java
View file
Original file line number Diff line number Diff line change
@@ -2550,7 +2550,7 @@ private static <M> String toString(Set<M> mods, String what) {
* Generates and returns a hashcode for the enum instances. The returned hashcode
* is a sum of each of the enum instances' {@link Enum#ordinal() ordinal} value.
*/
private static int ordinalHashCode(final Iterable<? extends Enum<?>> enums) {
private static int ordinalHashCode(Iterable<? extends Enum<?>> enums) {
int h = 0;
for (Enum<?> e : enums) {
h += e.ordinal();
Copy link
Contributor

Choose a reason for hiding this comment

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

This e == null check suggests there is an issue elsewhere, can you explain the scenario where you see this? Also can you drop the spurious use of "final" here, it's inconsistent with the existing code.

Do you want us to look at the test? It looks like it needs clean-up and I'm not sure if you want to wait for the CDS issue or not.

Copy link
Member Author

@jaikiran jaikiran Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hello Alan,

This e == null check suggests there is an issue elsewhere, can you explain the scenario where you see this?

That was just a pre-emptive defensive check I had added. There isn't a scenario in that flow where any of the elements are currently null. I have updated this PR to remove that check. Test continues to pass with this change. Also removed the "final".

Do you want us to look at the test? It looks like it needs clean-up and I'm not sure if you want to wait for the CDS issue or not.

Yes please. The test is ready for review. The only place where there's a TODO is where that part of commented code is affected by the bug mentioned in that comment. I would like to use that check (the commented out one) but I don't want to wait for that other bug to be fixed for this PR, since I am unsure how big or how long it might take for the other bug to be fixed. I plan to uncomment that part in a separate PR once that other bug is fixed, if that's OK.

Copy link
Member Author

@jaikiran jaikiran Oct 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hello Alan,

Looking at the CDS issue that's being tracked at https://bugs.openjdk.java.net/browse/JDK-8275731, it's looking like a much bigger change and might take a while. In the meantime do you think this test case (and the fix to the hashCode() part) looks OK? I am open to deleting the commented out equality check in this test case since although that equality testing should be done, it doesn't have to be done as part of this hashCode() fix/test PR. Let me know what you and others prefer.