Skip to content

Less method invalidations for OffsetInteger#280

Open
SimonDanisch wants to merge 3 commits into
masterfrom
sd/invalidations
Open

Less method invalidations for OffsetInteger#280
SimonDanisch wants to merge 3 commits into
masterfrom
sd/invalidations

Conversation

@SimonDanisch

@SimonDanisch SimonDanisch commented Jun 12, 2026

Copy link
Copy Markdown
Member

Back in the day, I didn't have enough time to figure out why OffsetIntegers invalidate so many methods and how to fix it. The answer is, just let promotion do the hard work, and dont define unused methods :D

…erflow

The mixed OffsetInteger/Integer methods for ==, >=, <=, <, > were
redundant: Base's promotion machinery (the promote_rule and convert
defined right above) produces identical semantics, exactly like mixed
+/-/* always worked here. Each of them invalidated compiled comparison
code in unrelated packages whenever GeometryBasics loads - ~1.8k method
instances in a Makie session, measured with SnoopCompile (e.g. an HTTP
server's whole listener chain recompiles because it contains an
abstractly-inferred `>`).

Also makes sub_with_overflow on OffsetInteger actually work: the macro
interpolation generated bodies calling an unqualified sub_with_overflow
that never resolved in this module, so every call has thrown an
UndefVarError since the code was written. Now defined (and qualified)
via Base.Checked; the mixed-Integer methods stay because Base has no
promoting fallback for it.
@ffreyer

ffreyer commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

This made me check if OffsetIntegers are tested and they are, but only with an offset of 0. I guess that just covers the first method and not both?
Either way, it would probably be good to extend the test to include some nonzero offset

@testset "Offsetintegers" begin
x = 1
@test GeometryBasics.raw(x) isa Int
@test GeometryBasics.value(x) == x
x = ZeroIndex(1)
@test eltype(x) == Int
x = OffsetInteger{0}(1)
@test typeof(x) == OffsetInteger{0,Int}
for x1 in [OffsetInteger{0}(2), 2, 0x02]
@test Base.to_index(x1) == 2
@test -(x1) == OffsetInteger{0,Int}(-2)
@test abs(x1) == OffsetInteger{0,Int}(2)
for x in [OffsetInteger{0}(1), 1, 0x01]
@test +(x, x1) == OffsetInteger{0,Int}(3)
@test *(x, x1) == OffsetInteger{0,Int}(2)
@test -(x, x1) == OffsetInteger{0,Int}(-1)
#test for /
@test div(x, x1) == OffsetInteger{0,Int}(0)
@test !==(x, x1)
@test !>=(x, x1)
@test <=(x, x1)
@test !>(x, x1)
@test <(x, x1)
end
end
end

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.

2 participants