On Tue, 31 Oct 2023 08:37:44 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> 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) Wow, I cannot recall ever getting bashed this hard for code of mine. Feel free to remove the Sentinel mechanism completely. It's not needed. I would keep the zero termination of the array. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15955#discussion_r1378486602