On Wed, 6 Dec 2023 02:14:13 GMT, David Holmes <dhol...@openjdk.org> wrote:
> I'm not following the changes to cdsHeapVerifier.cpp. You've marked the new > entries as `C` but the definition is: > > ``` > // [C] A non-final static string that is assigned a string literal during > class > // initialization; this string is never changed during -Xshare:dump. > ``` > > and these are final static strings not non-final. ??? I simply followed the fix to https://bugs.openjdk.org/browse/JDK-8295295 where the last time I added a `StaticProperty`, it broke the CDS test. Since I am not familiar in this area, I am happy to have the reasoning corrected. I would appreciate suggestions. > You also have a large number of test failures with this PR. Are you referring to GHA tests? I am not sure they are relevant to this fix, as my run for mach5 did not cause any regression test failures. > Can I also suggest that you change the bug and PR synopsis to say > StaticProperty rather than static properties as I found it quite confusing to > understand the issue. Modified. >> Making them static properties is safer than relying on the class >> initialization timing > "class initialization" refers to the static initialization of a class. I > assume that is not what you mean here but the creation of the instance of the > class? Even StaticProperty still initializes these things during class > initialization, so I'm unclear exactly what this is trying to say. Modified it to specifically saying `Locale` class loading time. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1843406352
