Less method invalidations for OffsetInteger#280
Open
SimonDanisch wants to merge 3 commits into
Open
Conversation
…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.
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? GeometryBasics.jl/test/runtests.jl Lines 313 to 341 in fcb4edf |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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