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

Update elliptic.nim #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update elliptic.nim #103

wants to merge 3 commits into from

Conversation

planetis-m
Copy link

The most significant change is turning let into const - a huge amount of temporaries goes away. Other is porting from strutils to strformat. And using 'bi when possible

@planetis-m
Copy link
Author

Error: undeclared identifier: 'bi'

@narimiran
Copy link
Member

Error: undeclared identifier: 'bi'

That's why we need https://github.com/nim-lang/bigints/blob/master/src/bigints/private/literals.nim, for example. See comment there: "It is needed as a workaround for Nim's parser for versions <= 1.4."

Long story short: please revert those 'bi changes.

@dlesnoff
Copy link
Contributor

dlesnoff commented Apr 3, 2022

So we have to drop support for versions <= 1.4 to define const litterals with Bigints ?
I appreciate the change from strutils to strformat, the echo looks nicer that way.

@konsumlamm
Copy link
Contributor

So we have to drop support for versions <= 1.4 to define const litterals with Bigints ?

We have to drop support for Nim <= 1.4 when we want to use 'bi.

Comment on lines 67 to 68
{(if publicKey[1] mod two == one: "03" & publicKey[0].toString(base = 16)
else: "02" & publicKey[0].toString(base = 16)):0>64}
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason the CI is failing is that this doesn't work in Nim 1.4.8, I think we'll have to use a variable instead.

@dlesnoff
Copy link
Contributor

dlesnoff commented Apr 4, 2022

I am not a fan of all those constants defined in each script. We should provide a file in the module with different constants, like zero, one, eventually the first digits up to ten, minus one, …
I don’t know if these declarations takes memory, but if it does, we should make these imports optional.

@planetis-m
Copy link
Author

Can you guys update the CI to 1.6 instead? Here and in different projects like the main repo, threading, etc my PRs are stalled because older versions of Nim don't have features needed.

@dlesnoff
Copy link
Contributor

dlesnoff commented Apr 4, 2022

I don’t think we will remove 1.4.8 support. It has been released on 26th May and I believe we keep support for Nim versions that has been released less than one year before (or at least two Nim versions).
Otherwise, I do not understand how this impact your personal projects : we do support 1.6.
Is it really a needed feature here ?
I think you can simply move the code between brackets into a let statement, before the echo.

@planetis-m
Copy link
Author

planetis-m commented Apr 4, 2022

No I meant various PRs I made in Nim are postponed because of build failures. Like nim-lang/Nim#19360 nim-lang/threading#6 This is pbl not a good place to discuss this.

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.

None yet

4 participants