Skip to content

Conversation

@carlopi
Copy link
Contributor

@carlopi carlopi commented Oct 9, 2025

Three independent fixes I bumped against while looking at a funky bug that is compiler specific (in particular gcc 14):

  • avoiding implicit conversions to uhugeint_t
  • pass const hugeint_t & instead of passing by value
  • remove the specialized comparison operators, that are redundant

Third 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.

@carlopi carlopi changed the title Hugeint_t fixes hugeint_t fixes Oct 9, 2025
Copy link
Collaborator

@lnkuiper lnkuiper left a 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

@duckdb-draftbot duckdb-draftbot marked this pull request as draft October 9, 2025 09:18
@carlopi carlopi marked this pull request as ready for review October 9, 2025 09:20
@duckdb-draftbot duckdb-draftbot marked this pull request as draft October 9, 2025 11:39
@carlopi carlopi marked this pull request as ready for review October 9, 2025 11:39
Copy link
Collaborator

@lnkuiper lnkuiper left a 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

@Mytherin Mytherin merged commit 46beeea into duckdb:v1.4-andium Oct 9, 2025
73 checks passed
@carlopi carlopi deleted the hugeint_t_fixes branch October 9, 2025 16:15
github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Oct 21, 2025
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Oct 21, 2025
hugeint_t fixes (duckdb/duckdb#19318)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Mytherin added a commit that referenced this pull request Dec 13, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants