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

Code review on UInt256 #1386

Closed
MithrilMan opened this issue Dec 30, 2019 · 1 comment · Fixed by #1387
Closed

Code review on UInt256 #1386

MithrilMan opened this issue Dec 30, 2019 · 1 comment · Fixed by #1387
Labels
Question Used in questions

Comments

@MithrilMan
Copy link

MithrilMan commented Dec 30, 2019

I've a question about your UInt256 implementation

neo/src/neo/UInt256.cs

Lines 15 to 33 in c5a0f07

private ulong value1;
private ulong value2;
private ulong value3;
private ulong value4;
public override int Size => Length;
public UInt256()
{
}
public unsafe UInt256(ReadOnlySpan<byte> value)
{
fixed (ulong* p = &value1)
{
Span<byte> dst = new Span<byte>(p, Length);
value[..Length].CopyTo(dst);
}
}

how can you be sure that value2, value3 and value4 are always pinned and have always the same contiguous memory ?
From what I see you create a pointer to cover 32byte of memory starting from the address of value1 that's the only pinned value. Do you have any documentation that ensure that the memory will be allocated sequentially for your 4 allocated ulong variables on every platform?
I've tried different UInt256 implementation and this is one of the most performant constructor but I've doubts about the correctness, can you enlighten me?
thanks

@MithrilMan MithrilMan added the Question Used in questions label Dec 30, 2019
@john-h-k
Copy link
Contributor

john-h-k commented Dec 30, 2019

Yeah, just add on [StructLayout(LayoutKind.Sequential)] to the class and this is immediately fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question Used in questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants