On Mon, 29 Mar 2021 21:35:52 GMT, Ioi Lam <ik...@openjdk.org> wrote:
> There are two versions of JVMFlagAccess::ccstrAtPut() for modifying JVM flags
> of the ccstr type (i.e., strings).
>
> - One version requires the caller to free the old value, but some callers
> don't do that (writeableFlags.cpp).
> - The other version frees the old value on behalf of the caller. However,
> this version is accessible only via FLAG_SET_XXX macros and is currently
> unused. So it's unclear whether it actually works.
>
> We should combine these two versions into a single function, fix problems in
> the callers, and add test cases. The old value should be freed automatically,
> because typically the caller isn't interested in the old value.
>
> Note that the FLAG_SET_XXX macros do not return the old value. Requiring the
> caller of FLAG_SET_XXX to free the old value would be tedious and error prone.
Hi Ioi,
This looks good to me. Thanks for fixing it up.
One minor nit below.
Thanks,
David
src/hotspot/share/services/writeableFlags.cpp line 250:
> 248: if (err == JVMFlag::SUCCESS) {
> 249: assert(value == NULL, "old value is freed automatically and not
> returned");
> 250: }
The whole block should be ifdef DEBUG.
-------------
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3254
