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.
I had a question but overall nice cleanup! src/hotspot/share/runtime/flags/debug_globals.hpp line 38: > 36: // have any MANAGEABLE flags of the ccstr type, but we really need to > 37: // make sure the implementation is correct (in terms of memory allocation) > 38: // just in case someone may add such a flag in the future. Could you have just added a develop flag to the manageable flags instead? src/hotspot/share/runtime/flags/jvmFlagAccess.cpp line 327: > 325: // The callers typically don't care what the old value is. > 326: // If the caller really wants to know the old value, read it (and make > a copy if necessary) > 327: // before calling this API. good comment! ------------- Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3254
