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

make endians support JS backend #16127

Closed
wants to merge 13 commits into from
Closed

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 25, 2020

I want to make oids module support JS backend. But it relies on endians module. Of course in JS/browser, it is always big endianess in Nim.

And nodejs backend does need endianess I think which is wrong now.

This PR will solve targetCPU nad targetOS:
https://github.com/nim-lang/Nim/pull/14543/files#diff-5bfae5b518d4ab360a8cd26e9d7fdfef1d24dbf9f9922c6f77d8660a23792f51R121

Or I need this to make oids support JS/browser backend

when not defined(js):
  import endians
else:
  proc bigEndian32(outp, inp: pointer) {.inline.} =
    # JS backend is always big endianess.
    cast[ptr uint32](outp)[] = cast[ptr uint32](inp)[]

  proc atomicInc(memLoc: var int, x: int = 1): int {.inline, discardable.} =
    inc(memLoc, x)
    result = memLoc

@ringabout ringabout marked this pull request as draft November 25, 2020 08:26
@ringabout ringabout marked this pull request as ready for review November 25, 2020 15:19
@ringabout
Copy link
Member Author

First make endians support JS backend.
Then rename builtin_bswap16, export that and deprecated original API.
Finally make nodejs backend support endians

proc bigEndian64*(outp, inp: pointer) {.inline.} = swapEndian64(outp, inp)
proc bigEndian32*(outp, inp: pointer) {.inline.} = swapEndian32(outp, inp)
proc bigEndian16*(outp, inp: pointer) {.inline.} = swapEndian16(outp, inp)
elif defined(js):
Copy link
Member

@timotheecour timotheecour Nov 25, 2020

Choose a reason for hiding this comment

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

can you remove this entire almost-duplicated block by making copyMem(outp, inp, k) work for js, so that there's no need to have elif defined(js): ?

=>

when useBuiltinSwap or defined(js):
...

@@ -11,39 +11,55 @@
## (`endian`:idx:).
##
## Unstable API.
when not defined(js):
Copy link
Member

Choose a reason for hiding this comment

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

remove 1 level of indentation:

when defined js:
 ...
elif when defined(gcc) or defined(llvm_gcc) or defined(clang):
 ...
elif ...

@ringabout ringabout marked this pull request as draft November 26, 2020 03:50
@ringabout
Copy link
Member Author

This PR is not correct(handling pointer wrongly), close it.

@ringabout ringabout closed this Nov 26, 2020
@timotheecour timotheecour added stale Staled PR/issues; remove the label after fixing them Javascript labels Nov 29, 2020
@timotheecour
Copy link
Member

Of course in JS/browser, it is always big endianess in Nim.

not true, little vs big endian, even on js, depends on architecture where code runs; but admittedly little endian will be much more common. refs timotheecour#515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Javascript stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants