jk-jeon / dragonbox

Reference implementation of Dragonbox in C++
Apache License 2.0
588 stars 37 forks source link

Conditional code fails due to static assertion of uint_least64_t being equal to unsigned long long #61

Closed libfud closed 4 months ago

libfud commented 5 months ago

https://github.com/jk-jeon/dragonbox/blob/bbb24df5229fcf81936259354a4a2d5f8f8d398b/include/dragonbox/dragonbox.h#L625

This is not the case on Fedora 39, 64 bit linux with clang 17. Unsigned long will work correctly in my case, but more testing is required depending on the platform rather than just the presence of __builtin_addcll.

jk-jeon commented 5 months ago

Thanks for bring this up. This PR is supposed to fix it, but the author seems to feel that it's not the best approach.

Are you using the lastest release, or the master branch one?

I am not sure what's the best way to fix it, but I will make a quick patch soon anyway.

Actually, the function is implicitly relying on the assumption that uint_least64_t is exactly 64-bit anyway, so I need a bigger fix for that.

libfud commented 5 months ago

I saw that PR afterwards. I'm not entirely sure that's the best way of handling it either. I think something like this could work:

#if JKJ_HAS_BUILTIN(__builtin_addcl) && JKJ_HAS_BUILTIN(__builtin_addcll)
                    inline void addCarry(unsigned long n) & noexcept {
                            unsigned long carry{};
                            low_ = __builtin_addcl(low_, n, 0, &carry);
                            high_ = __builtin_addcl(high_, 0, carry, &carry);
                    }

                    inline void addCarry(unsigned long long n) & noexcept {
                            unsigned long long carry{};
                            low_ = __builtin_addcll(low_, n, 0, &carry);
                            high_ = __builtin_addcll(high_, 0, carry, &carry);
                    }
#endif

                    JKJ_CONSTEXPR20 uint128& operator+=(stdr::uint_least64_t n) & noexcept {
                        auto const generic_impl = [&] {
                            auto const sum = (low_ + n) & UINT64_C(0xffffffffffffffff);
                            high_ += (sum < low_ ? 1 : 0);
                            low_ = sum;
                        };
                        // To suppress warning.
                        static_cast<void>(generic_impl);

                        JKJ_IF_CONSTEVAL {
                            generic_impl();
                            return *this;
                        }
#if JKJ_HAS_BUILTIN(__builtin_addcl)
                        addCarry(n);
#elif defined(_MSC_VER) && defined(_M_X64)

But that doesn't address uint_least64_t being exactly 64 bit. What exactly is the issue with uint_least64_t not being exactly 64 bit?

jk-jeon commented 5 months ago

Your suggestion seems to guide me into the right direction, thanks.

The issue of uint_least64_t is that, even if uint64_least_t has more than 64-bits, we are supposed to utilize only the lower 64-bits. So the carry should be added to high_ if and only if the result of addition n + low_ is larger than or equal to 2^64, which may or may not generate the hardware carry bit depending on the actual bit-width of uint_least64_t.

To be clear, this is not really a practical issue, as nobody (at least not I) cares about those weird platforms where uint_least64_t exists but is not exactly of 64-bits. But I just want to make sure the code is as standard compliant as possible.

libfud commented 5 months ago

I think in that case it's reasonable to document the assumption, include a static assertion to that effect, and note that it is the responsibility of someone porting this to their platform to correct that deficiency if it is the case on their platform.

jk-jeon commented 4 months ago

I think I resolved this issue. Please feel free to reopen it if you have a better suggestion. Thanks!