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. I can agree with the reasoning (except backporting, that can be applied to any change touching an existing file), it is arbitrary and depends where it is draw. It didn't take very long to do this PR, so it doesn't particularly matter to me. ------------- PR: https://git.openjdk.org/jdk/pull/12454
