On Fri, 22 Oct 2021 10:33:42 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 2559: >> >>> 2557: continue; >>> 2558: } >>> 2559: h += e.ordinal(); >> >> 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. > > 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. 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/6078
