On Mon, 17 Mar 2025 08:05:54 GMT, Volkan Yazici <[email protected]> wrote:
>> 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.
@vy thanks for taking feedback into consideration
the updated method names
initVectorMask, applyVectorMask, applyPlainMask
look like they "are on the money". 👍
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24033#issuecomment-2733679310
