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

Revert adding generic V: Ordinal parameter to succ, pred, inc, dec #22328

Merged
merged 3 commits into from
Aug 5, 2023

Conversation

konsumlamm
Copy link
Contributor

This was added in #20829, however it wasn't mentioned anywhere. Provided this passes the CI, I don't see why this was changed in the first place. I don't think it makes sense to add/subtract arbitrary Ordinals (which is what succ/pred currently allow). It also makes codegen more complicated (at least for JS, which is currently broken because of this, I haven't checked for C/C++), since now we can't rely on the second argument being an int anymore.

cc @bung87

@konsumlamm
Copy link
Contributor Author

konsumlamm commented Jul 25, 2023

For some reason I get an assertion failure for the runnableExamples in default, which seems to be unrelated:

proc default*[T](_: typedesc[T]): T {.magic: "Default", noSideEffect.} =
  ## Returns the default value of the type `T`. Contrary to `zeroDefault`, it takes default fields
  ## of an object into consideration.
  ##
  ## See also:
  ## * `zeroDefault <#zeroDefault,typedesc[T]>`_
  ##
  runnableExamples:
    assert (int, float).default == (0, 0.0)
    type Foo = object
      a: range[2..6]
    var x = Foo.default
    assert x.a == 2

On my fork, the CI doesn't fail.

@konsumlamm konsumlamm mentioned this pull request Jul 25, 2023
@konsumlamm konsumlamm marked this pull request as draft July 25, 2023 18:28
@konsumlamm
Copy link
Contributor Author

Ok, so this change causes default(range[2..6]) to be 0 instead of 2, I'll try to figure out why.

@ringabout
Copy link
Member

Ok, so this change causes default(range[2..6]) to be 0 instead of 2, I'll try to figure out why.

It's unrelated. Fixed by #22331

@konsumlamm konsumlamm marked this pull request as ready for review July 26, 2023 15:39
@konsumlamm
Copy link
Contributor Author

The CI failure seems to be unrelated.

@bung87
Copy link
Collaborator

bung87 commented Aug 5, 2023

just curious, is it suitable change these int32 to int?

@konsumlamm
Copy link
Contributor Author

just curious, is it suitable change these int32 to int?

Yes, trailingZeros2Digits always returns 0, 1, or 2 and tz and nd are always smaller than 30.

@bung87 bung87 merged commit e15e193 into nim-lang:devel Aug 5, 2023
19 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from e15e193

Hint: mm: orc; opt: speed; options: -d:release
168493 lines; 8.687s; 611.668MiB peakmem

@konsumlamm konsumlamm deleted the revert-ordinal branch August 5, 2023 16:53
ringabout added a commit to nim-lang/checksums that referenced this pull request Aug 11, 2023
@Araq
Copy link
Member

Araq commented Aug 11, 2023

Since this breaks our own checksums package this needs to be reverted.

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.

4 participants