RFR: 8267098: AArch64: C1 StubFrames end confusingly

[Andrew Haley]

On Mon, 17 May 2021 02:38:59 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

> > There's an urge from some contributors: when I suggest doing something in an easy-to-understand and clean way, people want to change everything else to match. This urge can be resisted, and IMVHO should be in this case. Churn is, in itself, bad.
> 
> Consistency is at least somewhat important though, right?

Certainly, yes. But please resist the temptation to do things in a less-good way to be consistent with existing code. Code that has been running solidly for a decade is a benefit too. It's a judgement call. Hard-and-fast rules are a Bad Thing, because they get in the way of carefully-considered judgement calls.

> This code is read much more often than it's modified, and in the case of the platform ports, often by people with less experience of OpenJDK as a whole. It seems worth spending a little time to do cleanups when modifying adjacent code, if it makes it easier to understand.

True, but bear in mind that people also read diffs; and sometimes you have to spend time trying to figure out if a diff was actually meant to change something.

> `dont_gc_arguments` is equally or more confusing because its value is false and then it gets passed to an argument `must_gc_arguments` whose sense is inverted. I don't see what's wrong with:
> 
> ```c++
> StubFrame f(sasm, "blah", /* must_gc_arguments */ false, /* does_not_return */ true);
> ```

The fact that you need comments to mark the arguments is a code smell too, IMO. Sure, if you're going to pass a bunch of booleans the comments are essential. But we are where we are.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4030