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

Add toBytes proc #128

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

Add toBytes proc #128

wants to merge 4 commits into from

Conversation

ehmry
Copy link
Contributor

@ehmry ehmry commented Jun 19, 2023

  • Export isNegative and isZero because it is easier to export here than to reimplement downstream.
  • Add toBytes proc for serializing. This is used by the CBOR package. Also easier to implement here than downstream.
  • Rename the tests/tliterals.nim to comply with Nimble standards. Not all tools are using nimble test for testing, some rely only on Nimble standards.

@dlesnoff
Copy link
Contributor

dlesnoff commented Jun 19, 2023

Thanks for your contribution! The algorithm looks fine to me.
– Could you add tests for the toBytes procedure?
– Could you add the corresponding fromBytes procedure?
I believe that the tliterals was renamed literals as to not run these tests. You might want to check the log for this specific file.

src/bigints.nim Outdated
Comment on lines 1309 to 1310
## Convert a `BigInt` to a byte-sequence.
## The byte-sequence *does not* include a sign-bit.
Copy link
Contributor

@dlesnoff dlesnoff Jun 19, 2023

Choose a reason for hiding this comment

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

Why this "limbs-only" bytes serialization?

Suggested change
## Convert a `BigInt` to a byte-sequence.
## The byte-sequence *does not* include a sign-bit.
## Convert the limbs of a `BigInt` to a byte-sequence.
## Sign-bit must be stored separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think sign-bits only belong in hardware-registers and it's more common for arbitrary width integers in binary serializations to be tagged some other way if they are negative. If a sign bit really needs to be in there then it can be inserted in the buffer after toBytes.

@ehmry
Copy link
Contributor Author

ehmry commented Jun 21, 2023

I­ pushed a test for toBytes. I will push fromBytes soon.

@ehmry
Copy link
Contributor Author

ehmry commented Jun 21, 2023

Now with fromBytes and a test of random serializations.

include tliterals
include ./literals
Copy link
Contributor

Choose a reason for hiding this comment

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

Why rename this specific file and not the others? Since we overwrite the default nimble test task, we shouldn't need the t prefix anymore. Either way, this is unrelated to the rest of the PR, so please make a new PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok then.

doAssert buf == @[0x1'u8,0x2,0x3,0x4,0x5,0x6,0x7,0x8,0xA]
if not a.isZero:
result = newSeq[byte](a.limbs.len shl 2)
var i: int
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var i: int
var i = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nim initializes integers to zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, but imo it's clearer to explicitly initialize them, rather than relying on implicit initialization.

var a, b: Bigint
for x in 0..trials:
a = rand(0.initBigInt..pow(2.initBigInt, x))
for endian in [bigEndian, littleEndian]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for endian in [bigEndian, littleEndian]:
for endian in [bigEndian, littleEndian]:

bigEndian and littleEndian aren't in scope here. I wonder why the CI didn't run...

@@ -880,6 +880,22 @@ proc main() =
doAssert pred(a, 3) == initBigInt(4)
doAssert succ(a, 3) == initBigInt(10)

when nimvm: discard
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this work in a static context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Endianness.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean std/endians is not available? I see.

for s in [24, 16, 8, 0]:
result[i] = uint8(a.limbs[a.limbs.high] shr s)
if result[0] != 0x00: inc(i)
for l in countdown(a.limbs.high.pred, a.limbs.low):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for l in countdown(a.limbs.high.pred, a.limbs.low):
for l in countdown(a.limbs.high.pred, 0):

low on a seq is always 0. You also used 0 below, so that's consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A low on an array is always 0, a low on an empty seq can be -1.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already checked that a is not zero (so a.limbs is not empty), but I don't really care, we can keep it that way.

var
li = result.limbs.low
bi = buf.low
while (bi + 4) < buf.len:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while (bi + 4) < buf.len:
while (bi + 3) < buf.len:

littleEndian32 uses buf[bi], buf[bi + 1], buf[bi + 2] and buf[bi + 3], not buf[bi + 4].

inc(li)
if bi < buf.len:
var
limb: uint32
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
limb: uint32
limb = 0'u32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nim initializes uint32 to zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit is better than implicit. For integers, I may understand since we implicitly rely on the litteral 0 being an integer.

bi = buf.low
block:
var
limb: uint32
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
limb: uint32
limb = 0'u32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nim initializes uint32 to zero.

Comment on lines +1320 to +1327
for s in [24, 16, 8, 0]:
result[i] = uint8(a.limbs[a.limbs.high] shr s)
if result[0] != 0x00: inc(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for s in [24, 16, 8, 0]:
result[i] = uint8(a.limbs[a.limbs.high] shr s)
if result[0] != 0x00: inc(i)
# convert a.limbs[a.limbs.high]
let highLimb = a.limbs[a.limbs.high]
for s in [24, 16, 8, 0]:
result[i] = uint8(highLimb shr s)
if result[0] != 0x00: inc(i)

Comment on lines +1350 to +1361
var
limb: uint32
j = 4 - (buf.len and 3)
while j < 4:
limb = (limb shl 8) or buf[bi].uint32
inc(bi)
inc(j)
Copy link
Contributor

@konsumlamm konsumlamm Jul 22, 2023

Choose a reason for hiding this comment

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

Suggested change
var
limb: uint32
j = 4 - (buf.len and 3)
while j < 4:
limb = (limb shl 8) or buf[bi].uint32
inc(bi)
inc(j)
var limb = 0'u32
for _ in 1..(buf.len and 0b11):
limb = (limb shl 8) or buf[bi].uint32
inc(bi)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(buf.len and 3) should be (buf.len and 0b11).

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, edited.

@konsumlamm
Copy link
Contributor

I'm against exporting isNegative and isZero. I think the better solution would be to support comparing with ints, then we can just use < 0 and == 0 (see #132).

@ehmry
Copy link
Contributor Author

ehmry commented Jul 23, 2023

I'm against exporting isNegative and isZero. I think the better solution would be to support comparing with ints, then we can just use < 0 and == 0 (see #132).

That would be adding more code to do something that is already implemented.

@konsumlamm
Copy link
Contributor

I'm against exporting isNegative and isZero. I think the better solution would be to support comparing with ints, then we can just use < 0 and == 0 (see #132).

That would be adding more code to do something that is already implemented.

Yes, but it's also uglier imo.

@dlesnoff
Copy link
Contributor

I am for exporting isNegative only and implement equality with integers.
Comparing with integers and fetching a boolean flag are two different things.

@konsumlamm
Copy link
Contributor

Note that isNegative really means "is less than or equal to zero", as 0 can also have isNegative set.

@dlesnoff
Copy link
Contributor

Thanks for the reminder.

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.

3 participants