-
Notifications
You must be signed in to change notification settings - Fork 39
Add openArray[char]
overloads to std/parseutils
#1544
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
base: devel
Are you sure you want to change the base?
Conversation
progress Fix bug and add doc comment links
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.
Thank you for the pull request.
As mentioned on Matrix, the CI failure is due to the csources compiler not supporting toOpenArray
at compile time. The csources compiler was updated fairly recently, and I personally think another update is not yet warranted.
I know that the start
parameter is largely redundant when the input is a slice, but please only use openArray
instead of string
for now. The start
parameter already has a default value, so at least the callsites aren't forced to use it.
Once s[x..y]
returns an openArray
(I personally think it should), the start
parameter can be removed, as the overhead of writing, e.g., parseHex(s[start..^1])
compared to parseHex(s, start)
is negligible, in my opinion.
|
||
proc nimParseBiggestFloat(s: string, number: var BiggestFloat, | ||
start = 0): int {.compilerproc.} = | ||
proc nimParseBiggestFloat(s: openArray[char], number: var BiggestFloat): int {.compilerproc.} = |
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 won't work. The procedure is a .compilerproc
, meaning that the compiler (usually the code generators) looks it up and emits a call to it internally, relying on the exact signature of the procedure.
Even if you update the places in the compile relying on the signature, the csources compiler won't know about this.
If you're interested, you could (in a separate PR) turn the procedure into a normal one, also removing the mParseBiggestFloat
magic in the process, which would allow for using openArray
as the parameter afterwards. Maybe there once was a good reason for the procedure to be magic, but I don't see why this is the case now.
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.
Does this not result with the same sort of issue, the bootstrapped compiler expects nimParseBiggestFloat
to be inside system
and marked with {.compilerproc.}
as such it will complain that it's missing nimParseBiggestFloat
if one removes the annotation?
I suppose to bootstrap the procedure could still be labelled a compiler proc until the csources are updated.
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.
I suppose to bootstrap the procedure could still be labelled a compiler proc until the csources are updated.
Yep, that's roughly what you'd do.
It's not exactly obvious, but this is how it'd work:
- keep the
nimParseBiggestFloat
procedures in.compilerproc
instrmantle
andjssys
- only enable the
parseBiggestFloat
magic implementation when booting (i.e.,when defined(nimKochBootstrap)
) - move the
nimParseBiggestFloat
implementation fromstrmantle
andjssys
intoparseutils.parseBiggestFloat
(only used when not in bootstrap mode) - add a VM hook for
parseBiggestFloat
and remove all remnants of the magic
Summary
Makes all the
std/parseutils
operations useopenArray[char]
.Keeps
string
overloads instd/parseutils
as they are still useful.Notes for Reviewers