ridiculousfish / libdivide

Official git repository for libdivide: optimized integer division
http://libdivide.com
Other
1.09k stars 77 forks source link

Regression: ptrdiff_t/size_t on macOS don't work anymore #96

Closed chris-se closed 2 years ago

chris-se commented 2 years ago

With commit https://github.com/ridiculousfish/libdivide/commit/623f9be3e406712be192e75833019f790aef8a51 support was added to make size_t work on some platforms, notably macOS.

This broke again with this commit: https://github.com/ridiculousfish/libdivide/commit/10ba9826646625101397c1203a9ca2622f14eba6

This is a regression in a codebase I'm working on when compiling on macOS, because it causes a compiler error in the following code:

libdivide::divider<std::ptrdiff_t> v{42};

This is because int64_t is aliased to long long on macOS, while ptrdiff_t is aliased to long (and size_t to unsigned long, while uint64_t is aliased to unsigned long long). And while both long and long long behave identically in that case, they are not the same type in the C++ type system, so there is no correct template that can be selected by the compiler for the libdivide::divider class.

On 64bit Linux this specific line of code isn't an issue since both ptrdiff_t and int64_t are aliased to long -- but there it is impossible to create a libdivide::divider<long long> object instead for the very same reasons.

Similarly, on Windows x64 both long and int are 32bit types, but long long is the corresponding 64bit type -- so either libdivide::divider<long> or libdivide::divider<int> will not work there, depending on which one is aliased to int32_t.

Is there a reason why the size-based dispatching introduced in the initial commit didn't work properly on AVR so that it had to be dropped for that?

adbancroft commented 2 years ago

Is there a reason why the size-based dispatching introduced in the initial commit didn't work properly on AVR so that it had to be dropped for that?

The AVR tool chain does not ship with a Standard Library, so no <type_traits> header.

I've reproduced the compile failure on Windows x64:

    // Issue #96: https://github.com/ridiculousfish/libdivide/issues/96
    libdivide::divider<short> short_div{42};
    libdivide::divider<int> int_div{42};
    libdivide::divider<long> long_div{42};
    libdivide::divider<long long> long_long_div{42};
    libdivide::divider<std::ptrdiff_t> ptr_diff_div{42};

Other than a compile test is there something else to test for?

chris-se commented 2 years ago

Ah, the lack of <type_traits> makes a lot of sense. (Though it's a bit unfortunate that they don't ship the bits of the STL that have zero runtime overhead.)

Other than a compile test is there something else to test for?

No, it either compiles and everything is fine, or it doesn't, and you immediately notice that. Your test for short/int/long/long long is perfectly sufficient here, because on all relevant platforms at least two of those types will have the same size and trigger this issue. (Though which two will vary.)

Many thanks for the extremely quick response and the fix!

chris-se commented 2 years ago

Also just tested your patch against v5.0 of libdivide and it works perfectly. Thanks!