On Mon, 25 Mar 2024 09:02:22 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
> > The only thing I'm uncertain about is the pData local, which I don't see
> > much benefit in removing since the null check associated with it still has
> > to remain for code semantics to be correct
>
> The point is that you can do the null check on the correct variable directly,
> instead of going a detour with pData. So instead of:
>
> ```
> RealType realVal;
> void* pData = getVal()
> if (pData == null) {
> // bail out
> }
> realVal = (RealType) pData;
> ```
>
> you can just do:
>
> ```
> RealType realVal = getVal();
> if (realVal == null) {
> // bail out
> }
> ```
>
> as the code normally would have been written, had there not been an old macro
> that used the "magic" temporary pData variable.
>
> This is a recurring pattern in this patch and needs to be fixed everywhere.
I have a concern since the null check bailout involves
THROW_NULL_PDATA_IF_NOT_DESTROYED, which is no longer accurate if we remove the
pData local. Maybe Phil can suggest an alternative to that, because I don't
know what that might be. Although, if given the choice I'd use RAII through
unique_ptr like Thomas suggested, it just seems so much cleaner to me. Speaking
of, hopefully this causes enough noise to get Phil's attention again. In the
meantime, I've reverted as much of the formatting changes I could possibly find
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019140763
