Обсуждение: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

John Naylor <john.naylor@enterprisedb.com> writes:
> It seems that we should have "Assert(word != 0);" at the top, which matches
> the other platforms anyway, so I'll add that.

That's basically equivalent to the existing Assert(non_zero).
I think it'd be okay to drop that one and instead have
the same Assert condition as other platforms, but having both
would be redundant.

I agree that adding the non-Assert test that Ranier wants
is entirely pointless.  If a caller did manage to violate
the asserted-for condition (and we don't have asserts on),
returning zero is not better than returning an unspecified
value.  If anything it might be worse, since it might not
lead to obvious misbehavior.

On the whole, there's not anything wrong with the code as-is.
A case could be made for making the MSVC asserts more like
other platforms, but it's quite cosmetic.

            regards, tom lane

John Naylor <john.naylor@enterprisedb.com> writes:
> On Sun, Jul 30, 2023 at 9:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That's basically equivalent to the existing Assert(non_zero).
>> I think it'd be okay to drop that one and instead have
>> the same Assert condition as other platforms, but having both
>> would be redundant.

> Works for me, so done that way for both forward and reverse variants. Since
> the return value is no longer checked in any builds, I thought about
> removing the variable containing it, but it seems best to leave it behind
> for clarity since these are not our functions.

Hmm, aren't you risking "variable is set but not used" warnings?
Personally I'd have made these like

    (void) _BitScanReverse(&result, word);

Anybody trying to understand this code is going to have to look up
the man page for _BitScanReverse anyway, so I'm not sure that
keeping the variable adds much readability.

However, if no version of MSVC actually issues such a warning,
it's moot.

            regards, tom lane