On Fri, 6 Feb 2026 12:21:43 GMT, Johan Sjölen <[email protected]> wrote:
>> Ioi Lam has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - restored assert
>> - @jdksjolen review comments
>
> src/hotspot/share/cds/aotCompressedPointers.hpp line 52:
>
>> 50: static narrowPtr cast_to_narrowPtr(T narrowp) {
>> 51: return checked_cast<narrowPtr>(narrowp);
>> 52: }
>
> This is an escape hatch that let's you encode anything into a `narrowPtr`,
> maybe it should be private? It doesn't seem to be used outside of the header
> (did a quick Ctrl+F).
Now I have only `cast_from_u4` and `cast_to_u4` and don't allow arbitrary types
anymore.
> src/hotspot/share/cds/aotCompressedPointers.hpp line 68:
>
>> 66: }
>> 67:
>> 68: static narrowPtr null_narrowPtr() {
>
> Nit: I think you can just call this `null` and that's fine and a bit
> prettier, but up to you.
Done.
> src/hotspot/share/cds/aotCompressedPointers.hpp line 103:
>
>> 101:
>> 102: template <typename T>
>> 103: static narrowPtr encode_null_or_address_cache(T ptr) { // may be null
>
> For everything else we have `X` and `X_not_null` but for this one we have `X`
> and `X_or_null`, so here we do the inverse. Is that on purpose?
I changed the name to `encode_address_in_cache_or_null()`.
I think only `encode_not_null()` and `encode_address_in_cache_or_null()` make
sense. Yes, one of them is `or` and the other is `not`. However, the opposite
doesn't work:
- If we used `encode_address_in_cache(p1)` and
`encode_address_in_cache_not_null(p2)` -- `p1` will never be null if it's
inside the cache, so the two functions are equivalent. Both `p1` and `p2`
cannot be null.
- If we used `encode(p1)` and `encode_or_null(p2)`, it's not clear that `p1`
must not be null.
> src/hotspot/share/cds/aotCompressedPointers.hpp line 145:
>
>> 143: return decode_not_null<T>(base_address, narrowp);
>> 144: }
>> 145: }
>
> We don't wanna do this with the `base_address` being an optional argument at
> the end instead so we just have one function def?
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29590#discussion_r2775591084
PR Review Comment: https://git.openjdk.org/jdk/pull/29590#discussion_r2775593035
PR Review Comment: https://git.openjdk.org/jdk/pull/29590#discussion_r2775619425
PR Review Comment: https://git.openjdk.org/jdk/pull/29590#discussion_r2775620252
