-
Notifications
You must be signed in to change notification settings - Fork 2.8k
hugeint_t fixes #19318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
hugeint_t fixes #19318
Conversation
lnkuiper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have a look at the inet test failures? It relies on an implicit conversion
8e60773 to
42f22f0
Compare
lnkuiper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! This is good to go once CI passes.
Thanks for looking into this bug in more detail. I'm happy the implicit conversion is gone now, this is a hazard
hugeint_t fixes (duckdb/duckdb#19318)
hugeint_t fixes (duckdb/duckdb#19318) Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
We used to use GCC 12 instead of the manylinux-default GCC 14 for compatibility reasons. This seems no longer required, and the GCC14 Arm bug with UUID has been resolved in #19318
Three independent fixes I bumped against while looking at a funky bug that is compiler specific (in particular gcc 14):
const hugeint_t &instead of passing by valueThird commit in particular makes the bug go away, and even on it's own it can not justify the behaviour, was creating a weird situation where in an abundance of available templates overloads compilers are not forced to pick one. On this consensus on compiler implemented evolved (see https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3606r0.html), but we better stay away from grey areas where different compilers might (somewhat reasonably) take different decisions.
The third commit alone solves #17757, at least I did check the simplest reproduction, should be uncontroversial changes that arguably would make sense in any case, so maybe I would consider this for merging in
v1.4-andium.