RFR: 8341696: C2: Non-fluid StringBuilder pattern bails out in OptoStringConcat [v4]

[Emanuel Peter]

On Wed, 8 Jan 2025 13:41:23 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

>> Hmm... Ok. In my opinion a `check` method should be "pure", so it shoud not really have a side-effect... but you do some
>> `sc->add_control(cnode);` and `sc->push_string(arg);` etc.
>> 
>> I'm not familiar enough with the code yet, but those side-effects. Ah, the comment in the hpp seems to suggest you `add` it. So maybe:
>> `add_the_append_candidate_to_sc_if_<something>`
>> A bit long, but would be more helpful I think
>
> I think then I will just call it `process_append_candidate` then. There's no point in trying to explain the complex logic in the method name. This method together with its callee would better be moved into another class anyhow – but I gave up this idea because of all the codependencies.

Ok, so if I understand this right, this fully "processes" the `add`.
That is why you call it `CheckAppendResult`. Then maybe the enum tags could be a bit more descriptive...
`GoodAppend` -> `AddedAppendToStringConcat`
`GiveUp` implies that the algo outside is supposed to give up... but then it does continue to do something out there... so who is giving up? Maybe there could be a better name here.
`NotAppend` -> does this mean it is not an append, so you do nothing? -> `DidNothingBecauseNotAppend`...

You probably have even better ideas. Good names are hard 🙈

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22537#discussion_r1907220250