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
system refactorings #10559
system refactorings #10559
Conversation
@@ -10,8 +10,8 @@ | |||
# Nim support for C/C++'s `wide strings`:idx:. This is part of the system | |||
# module! Do not import it directly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the system ...
Do not import it directly!
can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
"\" to preallocate sufficient storage." | ||
|
||
c_fprintf(cstderr, """too large thread local storage size requested, | ||
use -d:\"nimTlsSize=X\" to setup even more or stop using unittest.nim""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or stop using unittest.nim
-
TLS usage can come from any other module not just unittest, that comment seems off
-
previous code was showing the relevant variables here;
nimThreadVarsSize()
andsizeof(ThreadLocalStorage)
; we should show these too in thec_fprintf
msg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was hacked together. ;-)
@@ -15,439 +15,5 @@ | |||
{.push debugger:off .} # the user does not want to trace a part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not remove all remaining code from here and put: {.deprecated: "use
import io".}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
type | ||
CFile {.importc: "FILE", header: "<stdio.h>", | ||
incompletestruct.} = object | ||
CFileStar* = ptr CFile ## The type representing a file handle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not s/CFileStar/CFilePtr
? seems more readable and it follows the existing nim convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, will change it.
else: | ||
discard c_fprintf(f, "%ld", i) | ||
|
||
proc close(f: CFileStar): cint {. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- why not follow existing convention, ie
c_fclose
(which was previous name in sysio.nim) - like you said yourself,
discardable
is evil; it also makes errors silently accepted
for reference here was the previous definition:
proc c_fclose(f: File): cint {.
importc: "fclose", header: "<stdio.h>".}
- same with other procs that gained a
discardable
in this PR, egc_fputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ENDB, I hacked it together.
|
||
proc cmp(x, y: string): int = | ||
when nimvm: | ||
when defined(nimscript): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when defined(nimscript) or nimvm:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't compile. :P
It is still system-dependent, but shorter strings should come sooner. See nim-lang#8930 and nim-lang#10559
No description provided.