RFR: 8282365: Consolidate and improve division by constant idealizations [v35]
Quan Anh Mai
qamai at openjdk.org
Tue Dec 12 16:09:06 UTC 2023
On Thu, 7 Dec 2023 22:26:54 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
>>
>> missing include
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 1107:
>
>> 1105: using U = std::make_unsigned_t<T>;
>> 1106: return (x >= 0) ? x : U(0) - U(x);
>> 1107: }
>
> I understand what this to change to ABS is doing, though it's not obvious. (Dodging overflow UB
> for -x when x is the minimum value of a signed integral type.) I'm not entirely sure that's a wise move.
>
> As written this will trigger `-Wconversion` warnings someday (maybe). static_casting the subtraction
> result to T will eliminate that concern.
>
> However, this is an API change. The previous definition worked for floating point types, while this
> change does not. (std::make_unsigned requires T be an integral or enum, but not bool, type.)
>
> I also don't understand why this change is part of this PR.
>
> So I'm inclined to say no to this change without some compelling rationale.
Sure I have reverted that also.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1424233574
More information about the hotspot-compiler-dev
mailing list