Skip to content

Feature/checked wrapping ops#24

Open
moigagoo wants to merge 20 commits into
developfrom
feature/checked_wrapping_ops
Open

Feature/checked wrapping ops#24
moigagoo wants to merge 20 commits into
developfrom
feature/checked_wrapping_ops

Conversation

@moigagoo
Copy link
Copy Markdown
Collaborator

@moigagoo moigagoo commented Mar 4, 2026

Added:

  • raisingAdd
  • raisingSub
  • wrappingAdd
  • wrappingSub

All ops are implemented only in pure Nim so far and use overflowing ops internally.

The point of raising ops is that they are guaranteed to raise OverflowDefect on signed and unsinged ints no matter what the compilation flags are. This distinguishes them from + and - in the stdlib.

The point of wrapping ops is that they are guaranteed to wrap for signed and unsigned ints but unlike overflowing ops they don't return an overflow flag.

@moigagoo moigagoo linked an issue Mar 4, 2026 that may be closed by this pull request
@moigagoo moigagoo marked this pull request as ready for review March 4, 2026 19:30
@moigagoo moigagoo marked this pull request as draft March 4, 2026 19:53
@moigagoo moigagoo marked this pull request as ready for review March 5, 2026 19:22
@moigagoo moigagoo requested review from arnetheduck and nitely March 5, 2026 19:29
@nitely
Copy link
Copy Markdown

nitely commented Mar 5, 2026

All ops are implemented only in pure Nim so far and use overflowing ops internally.

it'd seem these can be defined as helpers in add/sub and so use the intrinsics when available. So why make them pure only?


oh nvm. It's so they can be called using pure.raising*/pure.wrapping* in user code and the rest will be added later, right?

@moigagoo
Copy link
Copy Markdown
Collaborator Author

moigagoo commented Mar 5, 2026

@nitely

it'd seem these can be defined as helpers in add/sub and so use the intrinsics when available. So why make them pure only?

Yeah, that sounds totally reasonable and I've thought about that at first. Like, if raising* is basically just overflowing* with an added if-else branch, it should be moved to composite where all the other sugar ops live.

I'm seeing problems with that:

  1. It's really weird to import raisingAdd and from composite, ain't nothing composite about that op. And renaming composite or creating another module just for raising feels like an overkill.
  2. I could move the implementation to ops/add.nim, i.e. make the raisingAdd dispatcher call overflowingAdd dispatcher and not an implementation from impl. This feels like like library design violation because dispatchers aren't meant to contain logic other then what implementation to call.
  3. Maybe raisingAdd isn't necessarily implemented with overflowinAdd? Maybe it's just the current pure Nim implementation that does it but other implementations could do it entirely differently or the pure Nim implementation will not use overflowingAdd in the future. For example, wrappingAdd used to rely on overflowingAdd but doesn't anymore.

oh nvm. It's so they can be called using pure.raising*/pure.wrapping* in user code and the rest will be added later, right?

Yeah, I'm basically following my own contributor's guide and existing architecture which separates dispatching from implementations and suggests adding the pure Nim implementation first.

@moigagoo moigagoo marked this pull request as draft March 5, 2026 20:15
@moigagoo
Copy link
Copy Markdown
Collaborator Author

moigagoo commented Mar 6, 2026

I did some experimentation with the lib structure but ended up following the current architecture.

For raisingAdd and raisingSub, I've added implementations with GCC intrinsics similarly to saturating*. In local benchmarks, they perform better, as expected.

@moigagoo moigagoo marked this pull request as ready for review March 6, 2026 11:28
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.

Ops: Add wrapping* and checked* flavors

2 participants