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.

Sign up for GitHub

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

Jump to bottom

sciter.Value.Int() should return int32 #286

Open
AshfordN opened this issue Feb 7, 2021 · 2 comments · May be fixed by #287
Open

sciter.Value.Int() should return int32 #286

AshfordN opened this issue Feb 7, 2021 · 2 comments · May be fixed by #287

Comments

@AshfordN
Copy link
Contributor

AshfordN commented Feb 7, 2021 •
edited
Loading

According to the docs, sciter integers are always 32-bit; however, sciter.Value.Int() returns int, which may be either 32-bit or 64-bit, depending on the system architecture (see here). This causes inconsistencies in the way negative numbers are parsed on different architectures. That is, -1 gets interpreted as 4294967295 on a 64-bit system; while it gets interpreted correctly (as -1) on a 32 bit system.
pravic added a commit that referenced this issue Feb 7, 2021
@pravic
fix: Value.Int should work with int32.
3b44840 
This applies to:

* `Value.Int() int32`
* `Value.SetInt(int32)`

refs #286
@pravic pravic linked a pull request Feb 7, 2021 that will close this issue
Open
pravic added a commit that referenced this issue Feb 7, 2021
@pravic
fix: Value.Assign should work with int32 and int64.
6146c79 
This applies to:

* `Value.Assign(int32)`
* `Value.Assign(uint32)`

* `Value.Assign(int64)`
* `Value.Assign(uint32)`

* `Value.Assign(int)`
* `Value.Assign(uint)`

refs #286
pravic added a commit that referenced this issue Feb 7, 2021
@pravic
fix: Value.Assign should work with int32 and int64.
77e1596 
This applies to:

* `Value.Assign(int32)`
* `Value.Assign(uint32)`

* `Value.Assign(int64)`
* `Value.Assign(uint32)`

* `Value.Assign(int)`
* `Value.Assign(uint)`

refs #286
pravic added a commit that referenced this issue Feb 7, 2021
@pravic
fix: Value.Assign should work with int32 and int64.
35090dc 
This applies to:

* `Value.Assign(int32)`
* `Value.Assign(uint32)`

* `Value.Assign(int64)`
* `Value.Assign(uint32)`

* `Value.Assign(int)`
* `Value.Assign(uint)`

refs #286
@pravic
Copy link
Member

pravic commented Feb 7, 2021

@AshfordN Well, I've fixed the Value.Int() and Value.SetInt().

However, what about Value.Assign(val interface{})? Currently, it accepts uint64 and just truncates it to uint32 (originally, to uint). Should we return an error instead? Or just not accept the uint64 as an interface?

@AshfordN
Copy link
Contributor Author

AshfordN commented Feb 7, 2021 •
edited
Loading

I think it's appropriate to do the truncation, as long you're confident it wouldn't be a source of confusion in the future. In my opinion, the sciter docs justifies that behavior and should make it obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Value.Int should work with int32 sciter-sdk/go-sciter
2 participants
@pravic @AshfordN