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

Improve calculation of JSON_INT_MAX #84

Closed
trevnorris opened this issue Aug 12, 2019 · 5 comments · Fixed by #115
Closed

Improve calculation of JSON_INT_MAX #84

trevnorris opened this issue Aug 12, 2019 · 5 comments · Fixed by #115
Milestone

Comments

@trevnorris
Copy link

trevnorris commented Aug 12, 2019

From https://github.com/udp/json-parser/blob/e6426ae/json.c#L49-L55. That value can be calculated mathematically using 2**((n * 8) - 1) - 1. Which can also be done using the following bit shift:

static const json_int_t JSON_INT_MAX = (1L << ((8 * sizeof(json_int_t) - 1))) - 1
@harkue
Copy link

harkue commented Nov 5, 2019

If Using

static const json_int_t JSON_INT_MAX = (1L << ((8 * sizeof(json_int_t) - 1))) - 1

It will trigger a warining such as:

json.c:48:1: warning: integer overflow in expression [-Woverflow]
 static const json_int_t JSON_INT_MAX = (1L << ((8 * sizeof(json_int_t) - 1))) - 1;
 ^~~~~~

@harkue
Copy link

harkue commented Nov 5, 2019

Using this will be better:

static const json_int_t JSON_INT_MAX = (json_int_t)((1UL<<((sizeof(json_int_t) * 8) - 1)) - 1);

@LB--
Copy link
Member

LB-- commented Aug 7, 2021

Looks like this is being addressed in #102

@LB-- LB-- added this to the v1.1.1 milestone Aug 14, 2021
@LB-- LB-- linked a pull request Aug 14, 2021 that will close this issue
@DimitriPapadopoulos
Copy link
Contributor

Would that work on a platform where long (and hence 1UL) is 32-bit, while json_int_t is defined (probably not in a portable way) to be 64-bit?

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Aug 14, 2021

What was wrong with the previous definition of JSON_INT_MAX reverted by fcfa748? It's not clear to me.

static const json_int_t JSON_INT_MAX = (((unsigned json_int_t)0 - (unsigned json_int_t)1) / (unsigned json_int_t)2);

Perhaps casting back to signed?

static const json_int_t JSON_INT_MAX = (json_int_t)(((unsigned json_int_t)0 - (unsigned json_int_t)1) / (unsigned json_int_t)2);

DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 16, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 16, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 18, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 18, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 19, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 29, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Aug 29, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Oct 26, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
LB-- added a commit to LB--/json-parser that referenced this issue Oct 31, 2021
Fixes json-parser#84
Closes json-parser#102
Probably affects json-parser#115

In theory this should be guaranteed to work regardless of the underlying representation: https://stackoverflow.com/a/2711560/1959975

Demos for C++17 and C89:
https://gcc.godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGe1wAyeAyYAHI%2BAEaYxBIAHKQADqgKhE4MHt6%2BekkpjgJBIeEsUTFc8XaYDmlCBEzEBBk%2Bfly2mPZ5DDV1BAVhkdFxtrX1jVktCsM9wX3FA2UAlLaoXsTI7BzmAMxYNCEA1ABq2ABKAJIAYgCaAPpuAIIBbhDaCgLXwQTXBKR7DPN7LzeHy%2Bvy2W2uAFk7gANCD/EwAdgse2ImAIKwYe2erwY70EX3mUC8DBSwBC6ABOLxnwI8wAtFw9gB6LHE0nkylA/G0szzEybZGIgAiJg0d1F4rMmzwVB2e2u12QCW8CmVXgUEq2wWQ3iwe35bloeBYhA1m2wmqlO2mhxOF0u2K5NJ%2Bf31AFY3BK9t7ObjgQRQVLwSwmKo4fqkSi0Ri9hN0CAQAwfNE8MhrkaTQQzW5AX7ufzsAmQ2G%2BQKI0L3Z6xT7bWcrrcHk9c9Svi6%2BWKtm0lJbtphdpha/bB/X7o8e64ZZqxUc6w7kAg6j953VS1Zp3arhAFAgSN9Yzv6quJTP7RAPj8Pkf17OILQBMAfnejFe7ifN0/gHsP4/7x%2BX2%2BHQVD4uDMeI9mA0CXw4RZaE4N1eD8DgtFIVBOE9SxrFjZZVgHLYeFIAhNGgxYAGsQE2AA2AA6LhaK4CiNA0Lg3ViN1NjKfROEkBCiJQzheAUEANAIojFjgWAkDQFgEjoaJyEoKSZPoGJgFosw%2BDoAhokEiAIl4iJgjqABPTh8KkthBAAeQYWgTKQ3gsBDIxxHs0h8FRKoADdMEE1zMFUSovC00zeA%2BNpeKNCJiGMjwsF4ghiGNELFioAxgAUA48EwAB3SyEkYEKZEEEQxHYKQivkJQ1F43QWgMIwUGsax9DwCJBMgRZUASDpfLpON%2BSFMkvCaywuARPY6UszYJoAdTEWhZv8hKmEW4hiBIAS2kqDoXAYdxPCafw9t6IoSmyZJUgEUZmkSC6OhO/pSladpqkma69AqKoBC6eoHtmJ6Jm6d7xkmP6zq4RZXhWNY9ASzB1h4GC4J41zUI4VRYgoukKMkPZgGQZA9loqizCxdCrEsH5cEIEh9SlFo9g8aTZOIOn2PmXhCPs%2BZSPIjQqLdQW3Q0CiKOF2IuE2YWKM4jhuNIFgJEY0hEOQtGBKEkTudIcTEBQVBmeU%2BSIEUlmQFUkCNNoLTiB0vTXIM5hiDssyDYsghrNs3jHMMYAXOQ9ztrwbzfOQ/zAuC7hQsEcLXMi6Lndi9ZkISpKo5StKMqy3L8sQ/D%2BGK0RxHKgvKpUdRXN0dT6uMEabEi9q4RQ7q0l6/rNkGpNYgATgmqbZvmxbVGW1b1tZulVD2ZBNpe5wIFcYHSECaZTrmW7cjSReckuhgwbXz6Oh%2BhoDrGZ6g%2B%2B0GV8ej63pPm7Ad%2Bq//okSHsJhlo4YR6DZfglXeLRjGWMcZ4wJkTLgJMyZ1ypvgIgrM8I/CZkpaIbNeSc1ErzTYmwqKYJwbgvBst5aq14OrWwmsuZaB5rLMwKM1b8S1hQxY3lbZpBAJIIAA%3D%3D
DimitriPapadopoulos added a commit to DimitriPapadopoulos/json-parser that referenced this issue Oct 31, 2021
Either it has been defined explictly, or we attempt to calculate it
using the method reverted by fcfa748.

See discussion in json-parser#84 and json-parser#102.
@LB-- LB-- removed a link to a pull request Nov 2, 2021
@LB-- LB-- linked a pull request Nov 2, 2021 that will close this issue
@LB-- LB-- closed this as completed in #115 Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants