Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve Int128 API #123

Merged
merged 8 commits into from
Mar 10, 2025
Merged

improve Int128 API #123

merged 8 commits into from
Mar 10, 2025

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Mar 7, 2025

Summary

  • make some small improvements to the Int128 API
  • make tester treat t*.nim files as tests
  • add tests for Int128

Details

API improvements:

  • < and <= now treat the operands as signed integers
  • unsigned comparisons are renamed to <=% and <% (in line with the
    standard library naming scheme)
  • rename / to div, to reserve the former for fractions
  • shl and shr now consistently wrap the shift distance
  • accept all integer types with toInt128, not just int

All usages of Int128 comparisons are adjusted, except for the ones
in builtin.nim, which should have been signed comparisons from the
start (this fixes an interpreter bug).

To easily run NimSkull-based tests, the tester now considers t*.nim
as tests. A set of tests for Int128 based on NimSkull's tint128.nim
is added to the test suite.


Notes For Reviewers

zerbina added 6 commits March 7, 2025 16:08
* `shl` now also accepts an integer
* `shr` now wraps the shift distance prior to shifting (same as what
  `shl` already does)
This is in line with the nomenclature used for the built-in integer
types, where `div` returns an integer (instead of a floating-point
number).
The standard `div` treats the values as signed, and for consistency,
`<` and `<=` now do too. All usage sites are updated.
They are based on the tests for the NimSkull compiler's internal
`Int128` type.
@zerbina zerbina added bug Something isn't working enhancement New feature or request labels Mar 7, 2025
@zerbina zerbina requested a review from saem March 7, 2025 16:42
@zerbina
Copy link
Collaborator Author

zerbina commented Mar 7, 2025

@saem: I'm not sure what licensing obligations taking and modifying the test file from NimSkull entails.

@saem
Copy link
Contributor

saem commented Mar 9, 2025

@saem: I'm not sure what licensing obligations taking and modifying the test file from NimSkull entails.

They're both MIT licensed, so that's fine, there is no explicitly copyright notice so that doesn't have to replicated (assuming an overlay literal reading). I think as is is fine.

Copy link
Contributor

@saem saem left a comment

Choose a reason for hiding this comment

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

One very minor consideration, but incredibly soft suggestion, feel free to ignore it as I'm not convinced it's better.

@@ -360,7 +360,8 @@ if file.len == 0:
# XXX: parallel execution of tests is still missing
for (dir, runner) in dirs.items:
for it in walkDir(dir, relative=false):
if it.path.endsWith(".test"):
if it.path.endsWith(".test") or
(it.path.endsWith(".nim") and it.path.extractFilename.startsWith("t")):
Copy link
Contributor

Choose a reason for hiding this comment

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

Other option would be to only do it for the unittest directory, treating it as a specific runner accommodation. This is fine though, just thinking out loud.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the longer term, I think every directory should specify (via some sort of configuration file) what files to consider tests itself, though there's also the real risk of turning the tester into some overly general and complex tool.

@zerbina zerbina merged commit c178839 into nim-works:main Mar 10, 2025
6 checks passed
@zerbina zerbina deleted the int128-improvements branch March 10, 2025 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants