[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8317132: Prepare HotSpot for permissive- [v9]
From:       Julian Waters <jwaters () openjdk ! org>
Date:       2023-10-31 8:40:33
Message-ID: lgZUf-KwkBt3kQaM9Y-ReS08WoGTDcFWKsC20q7FVxQ=.a0a0fb1b-d9fe-43d5-a2f2-899fcdd10575 () github ! com
[Download RAW message or body]

On Tue, 31 Oct 2023 07:32:35 GMT, Kim Barrett <kbarrett@openjdk.org> wrote:

> > I know he was away until October but not exactly when in October.
> 
> Ick! I hadn't followed https://github.com/openjdk/jdk/pull/15096 closely, so
> hadn't noticed the discussion there.  Not sure why this is using a literal 0
> rather than `(T) '\n'` as in that PR?
> 
> So the literal "0" (or previously, NUL char constant) is either an integral
> zero or a null pointer constant (because one of the types used for T is
> HMODULE). imprint_sentinal uses `(T)'X'`. (Double ick! The value is either an
> integral constant or a HMODULE with a weird value.)
> 
> An approach to being type safe/consistent would be to provide a traits utility
> for specifying the terminator and the sentinal on a type-specific basis. Don't
> know if that's worth doing in this PR. We've been living (dangerously?) with
> the existing mechanism for a long time.
> 
> Something like
> 
> template<typename T>
> struct SimpleBufferSpecialValues;
> 
> template<>
> struct SimpleBufferSpecialValues<char> {
> char terminator() const { return '\0'; }
> char sentinal() const { return '\X'; }
> };
> 
> template<>
> struct SimpleBufferSpecialValues<HMODULE> {
> HMODULE terminator() const { return nullptr; }
> HMODULE sentinal() const {
> // Untested, might be completely wrong.
> alignas(HMODULE) static const char sentinal_data[1];
> return reinterpret_cast<HMODULE>(&sentinal_data);
> }
> };

I'd felt that the literal zero fit better, because it avoided an ambiguous cast and \
could fit in the context of a null pointer constant and char value of 0 both, casting \
a char constant to a pointer seemed to be something to avoid. The HMODULE or char in \
imprint_sentinel would always have a value of 88, which I'm not too fond of either \
(particularly in the case of HMODULE, which doesn't seem right since 88 is a \
perfectly normal value for a HMODULE to have, doesn't seem to fit the role of \
sentinel value too well)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15955#discussion_r1377224772


[prev in list] [next in list] [prev in thread] [next in thread]