On Tue, 7 Feb 2023 14:43:01 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Justin King has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix refactor mistake >> >> Signed-off-by: Justin King <jck...@google.com> > > I think also having a PR with a bullet list is a good sign that you're doing > too much in one change. I agree with @coleenp, sorry. I think this PR is premature. My advice would be to discuss such invasive changes on the ML first before opening a PR. Sometimes we do broad changes like this to keep the code base fresh. But it's always a tradeoff. Often the benefits of correcting the code base outweigh the cost, so we do it. But sometimes they don't. And I argue that in this case, they don't. Mental load cost: Not every name in the hotspot adheres to our naming guides, but may nevertheless be burnt into the collective brain of developers who have worked with the code base for many years. We talked about them, argued about them, there are tons of ML discussions and private emails and documentation surrounding them, they appear in scripts and test documentation... I'd hate to talk about the flag previously known as MEMFLAGS. It is a dividing line, and necessarily an arbitrary one. Not everyone sees this line at the same point. For a broad change that lies on this side of the line see the recent NULL->nullptr changes. Invasive, sure, but useful enough to do. Backporting cost: three LTS releases are still maintained by vendors, soon to be joined by a fourth one. Such a broad change makes backporting fixes difficult. The NULL->nullptr change was simpler in that regard because even though it spoils automatic merges, merging manually is straightforward. But renaming is a different matter. We have seen such problems in the past with Metaspace class renames, and that is just an isolated subsystem. Moreover, with MEMFLAGS, things are in flux; we may change the implementation in the future, and there are different plans for it. So I'd leave this as it is. ------------- PR: https://git.openjdk.org/jdk/pull/12454
