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

[WIP] fix #13790 #13792

Closed
wants to merge 16 commits into from
Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 28, 2020

WIP, stay tuned

TODO

  • fix tests
  • maybe add --legacy:implicitPtrCharToCstring
  • maybe generate warning instead of error for now

TODO after PR

  • get completely rid of nimNoArrayToCstringConversion branches in code (but leave the symbol defined) which was introduced in 3308d26; it was introduced in 2017 so not needed for bootstrap anymore and it will simplify a lot of code; that nimNoArrayToCstringConversion was in same spirit as this PR and is related
    (EDIT: i did that in another PR in the meantime)

Copy link
Contributor

@krux02 krux02 left a comment

Choose a reason for hiding this comment

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

Conversions are done with cstring(expr) or cast[cstring](expr). Don't introduce new identifiers for conversion.

lib/system.nim Outdated
Comment on lines 261 to 315
template toCstring*(a: ptr char): cstring = cast[cstring](a)
template toCstring*[N](a: ptr array[N, char]): cstring = cast[cstring](a)
template toCstring*[N](a: array[N, char]): cstring = cast[cstring](a.addr)
template toCstring*[N](a: ptr UncheckedArray[char]): cstring = cast[cstring](a)
template toCstring*(a: UncheckedArray[char]): cstring = cast[cstring](a.addr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all of this. Do not add new stuff to system.nim unless it is required. And if it is required, justify why it needs to be in system.nim.

@@ -915,7 +915,7 @@ proc getSymbol(L: var TLexer, tok: var TToken) =
proc endOperator(L: var TLexer, tok: var TToken, pos: int,
hash: Hash) {.inline.} =
var h = !$hash
tok.ident = L.cache.getIdent(addr(L.buf[L.bufpos]), pos - L.bufpos, h)
tok.ident = L.cache.getIdent(addr(L.buf[L.bufpos]).toCstring, pos - L.bufpos, h)
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
tok.ident = L.cache.getIdent(addr(L.buf[L.bufpos]).toCstring, pos - L.bufpos, h)
tok.ident = L.cache.getIdent(L.buf[L.bufpos].addr.toCstring, pos - L.bufpos, h)

to be consistent with the line in 904. Also I agree with krux02 here, instead of toCstring you should use casts manually and remove toCstring.

Comment on lines +937 to +1035
proc setLen*(s: var string, newlen: Natural, isInit = true) {.
magic: "SetLengthStr", noSideEffect.}
## Sets the length of string `s` to `newlen`.
## Sets the length of string `s` to `newlen`. If `isInit == true` and
## If `newlen > s.len`, when `isInit == true`, new entries are '\0' including
## the unreachable terminator n[s.len]. when `isInit == false`, only the
## terminator is '\0' (for optimization). TODO: implement this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't change setLen's semantics in this PR.

@@ -1,3 +1,5 @@
const notJSnotNims = not defined(js) and not defined(nimscript)
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline this const please

Comment on lines 27 to 93
proc strAppend*(result: var string, a: ptr char, n: int) {.inline.} =
## appends `n` `char`'s from `a` to `result`, efficiently
## note: should use a Slice[char]
# D20200328T022947
let old = result.len
result.setLen(old + n, isInit = false) # optimized here
when notJSnotNims:
copyMem(result[old].addr.toCstring, a, n)
else:
let a2 = cast[cstring](a)
for i in 0..<n: result[old+i] = a2[i]

proc strAppend*(result: var string, a: cstring) {.inline.} =
# TODO: replace `CStrToStr` ?
strAppend(result, cast[ptr char](a.unsafeAddr), a.len)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't belong in this PR either.

lib/system/repr.nim Outdated Show resolved Hide resolved
return $buf
const N = (pointer.sizeof * 2) + 2 + 1 # 0x + hex addr + '\0'
var buf {.noinit.}: array[N, char]
result.strAppend buf[0].addr, c_sprintf(buf.toCstring, "%p", x)
Copy link
Contributor

@Clyybber Clyybber Mar 28, 2020

Choose a reason for hiding this comment

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

Suggested change
result.strAppend buf[0].addr, c_sprintf(buf.toCstring, "%p", x)
discard c_sprintf(buf, "%p", x)
return $buf

As strAppend really doesn't belong in this PR

Copy link
Contributor

@Clyybber Clyybber left a comment

Choose a reason for hiding this comment

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

Agree with krux02 in that the toCstring templates should be inlined and removed from system.nim
Also please remove the unrelated strAppend and the setLen change from this PR.
Other than that nice :)

## append `n` `char`'s from `a` to `result`, efficiently;
## `\0` are insignificant.
## note: should use a Slice[char]
# D20200328T022947
Copy link
Collaborator

@juancarlospaco juancarlospaco Mar 29, 2020

Choose a reason for hiding this comment

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

?

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Mar 29, 2020

I do not understand why all those templates need to be public,
even template names are longer than its code body itself.

@stale
Copy link

stale bot commented Apr 1, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 1, 2021
@stale stale bot removed the stale Staled PR/issues; remove the label after fixing them label Apr 16, 2021
@stale
Copy link

stale bot commented Apr 17, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 17, 2022
@stale stale bot closed this May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ptr char implicitly converts to cstring, resulting in undefined behavior
4 participants