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

Replace unsafeAddr by baseAddr #32

Merged
merged 8 commits into from
Nov 24, 2022
Merged

Replace unsafeAddr by baseAddr #32

merged 8 commits into from
Nov 24, 2022

Conversation

lchenut
Copy link
Contributor

@lchenut lchenut commented Aug 3, 2022

It should fix some of the https://github.com/status-im/nim-protobuf-serialization CI problems.
The rest of the problems should be solved with #31

@arnetheduck
Copy link
Member

hm, I'm not convinced baseAddr is well-defined for empty arrays - taking the address of an empty array is not meaningful really - I guess using baseAddr points to these places more clearly, but we should probably revisit this at some point cc @zah (the risk being that with baseAddr, we turn a Defect into runtime-undetected UB).

The alternative solution here might be to avoid copyMem using something like assign2.

@@ -335,7 +335,7 @@ template writeByte*(span: var PageSpan, val: byte) =

template charsToBytes*(chars: openArray[char]): untyped =
Copy link
Member

Choose a reason for hiding this comment

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

@arnetheduck
Copy link
Member

should also add a test for writing/finalizing with empty array

@@ -335,7 +335,7 @@ template writeByte*(span: var PageSpan, val: byte) =

template charsToBytes*(chars: openArray[char]): untyped =
bind makeOpenArray
var charsStart = unsafeAddr chars[0]
Copy link
Member

Choose a reason for hiding this comment

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

baseAddr has been fixed to return ptr T (so the ptr byte cast is no longer necessary) and to return nil when the source is empty, so this code will now be as safe as it gets

@arnetheduck
Copy link
Member

devel failure unrelated cc @zah

@arnetheduck arnetheduck merged commit b42daf4 into master Nov 24, 2022
@arnetheduck arnetheduck deleted the remove-unsafeaddr branch November 24, 2022 11:55
@bung87
Copy link

bung87 commented Jan 4, 2023

where is baseAddr defined ? I get .nimble\pkgs2\faststreams-0.3.0-62f7ac8fb200a8ecb9e6c63f5553a7dad66ae613\faststreams\outputs.nim(535, 34) Error: undeclared identifier: 'baseAddr'

nim-1.6.10

@arnetheduck
Copy link
Member

it's part of nim-stew in ptrops - likely, you're using some old version

@bung87
Copy link

bung87 commented Jan 4, 2023

really? it seems all the status-im packages my project relys has no tag, so no version restrictions, I also checked nimble packages dir, all of them match the latest version declared in nimble file. it seems template symbol resolution problem to me.

@bung87
Copy link

bung87 commented Jan 4, 2023

hmm, you're right, when not versioned nimble not trying getting the latest.

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