RFR: 8267098: AArch64: C1 StubFrames end confusingly

[Andrew Haley]

On Tue, 18 May 2021 09:02:59 GMT, Andrew Haley <aph 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? 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.
>> 
>>> 
>>> And this case, is special, I think, because `does_not_return` uses the "don't do" anti-pattern, where the `true` case was `does_not_return`.
>> 
>> `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);
>
>> > 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.

Bear in mind also that this patch adds some complexity but provides (almost) no benefit to users. It's pretty marginal as a change, and therefore it's hard to justify much churn.

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

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