Skip to content

Fix Numbers.Int_base.compare - #13606

Merged
lthls merged 2 commits into
ocaml:trunkfrom
lthls:fix-number-comparison
Nov 13, 2024
Merged

lthls merged 2 commits into
ocaml:trunkfrom
lthls:fix-number-comparison

Conversation

@lthls

@lthls lthls commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

The compare function used for integers in (parts of) the compiler is wrong (i.e. not suitable for use as argument of Map.Make). This eventually caused bugs in the flambda-backend compiler, which was building maps with potentially negative numbers as keys (see oxcaml/oxcaml#3268 for the same fix there).

@gasche gasche left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine.

Note: unless I am missing something, the trunk code should be correct also with negative integers, as long as there is no overflow during the computation. So the problem should be with really large inputs (in absolute value), for example if one of the values is min_int.

@lthls
lthls merged commit 542e34f into ocaml:trunk Nov 13, 2024
@bclement-ocp

Copy link
Copy Markdown
Contributor

Note: unless I am missing something, the trunk code should be correct also with negative integers, as long as there is no overflow during the computation. So the problem should be with really large inputs (in absolute value), for example if one of the values is min_int.

I believe the point is rather that the code is correct if both inputs are nonnegative so you need at least one negative value (which does not need to be of high magnitude, if the nonnegative input is itself large) to trigger a bug. For context the issue originally occurred when using (large and potentially negative) hashes as keys.

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