On Fri, 14 Mar 2025 15:13:52 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Make method name changes backport-friendly
>
> persisting with method nomenclature (or painting the bikeshed again). What’s
> in a name?
>
> reset is really init() or setMask()
>
> Currently you have a static applyMask() method and a non static
> applyMask() — is that ideal ?
>
> The Galloping mask methods, there has to be a more semantically meaningful
> name e.g. applyLongMaskRepeating or applyVectorMaskRepeating
> (on your horse rather than on yer bike ;-)
>
>
> I still think the following looking meaningful
>
> public void applyMask(ByteBuffer src, ByteBuffer dst, theMask) {
> if (src.order() == dst.order()) {
> initLongVectorMask(src, dst, theMask);
> applyLongVectorMask(src, dst, theMask);
> }
> endApplyMask(src, dst, theMask);
> }
@msheppar, thanks so much for taking time to re-review the changes. In
bb81642acf10159b81f45e3260448a509f4d64c9, I've incorporated your feedback with
following `Masker` method name changes:
* `reset` -> `setMask`
* `initGallopingMask` -> `initVectorMask`
* `applyGallopingMask` -> `applyVectorMask`
I did not use the term `long` in the method name, since it is an internal
implementation detail. Vectorization can be implemented in several different
ways. The important bit here is the fact that we have two masking routine
variants: _plain_ and _vectorized_, and this now we capture in method names.
I also didn't want to use the term `end` in the method name, because it doesn't
_end_ anything – it is simply _plain_ masking without any catches. I could have
used `applyByteMask`, but again, this one-`byte`-at-a-time operation is an
implementation detail, which I did not want to disclose in the method name.
> Currently you have a static `applyMask()` method and a non static
> `applyMask()` — is that ideal ?
No, it is not. I liked your earlier idea of `Masker` creating a state object
that the call-site can pass to masking methods. There are even further
optimizations we can carry out – e.g., `MessageDecoder` is not re-using
`Masker` instances, whereas `MessageEncoder` does. But as discussed earlier, we
want to keep the scope limited with the fix to the problem at hand without
disrupting the code base much.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24033#issuecomment-2728513453
