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

num_digits: double → int #136

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Aug 28, 2021

It looks like a bad idea to apply operators ++ and -- to a double, or to compare a double to integer values.

@LB--
Copy link
Member

LB-- commented Sep 3, 2021

Interesting, I wonder why it was ever a double to begin with. Do you think we need to cast it to a double again during the pow call near line 384?

top->u.dbl += num_fraction / pow (10.0, num_digits);

I get the feeling there may be circumstances where the call could be ambiguous without the cast, though perhaps I am wrong. Maybe keeping it as an int would benefit the code instead. Thoughts?

@DimitriPapadopoulos
Copy link
Contributor Author

The call to pow() will implicitly cast to a double anyway:

double pow(double x, double y);

We could make the cast explicit, but I don't see any reason to do that.

@LB--
Copy link
Member

LB-- commented Oct 30, 2021

When compiling as C++, pow might have numerous overloads even without the std:: prefix, which is why I was concerned. Though, this might technically only be an issue in non-conforming implementations, I can't remember if unqualified pow is required to be unambiguous in C++.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Oct 31, 2021

The C++ pow() with overloads would be in <cmath>, wouldn't it?

We include <math.h> which defines pow() as:

double pow (double base, double exponent);

https://www.cplusplus.com/reference/cmath/pow/

It looks like a bad idea to compare apply to a double operators ++ and --,
or to compare to integers.
@LB--
Copy link
Member

LB-- commented Nov 2, 2021

Ah, right you are! Somehow of all things I considered, I forgot <math.h> is being used and not <cmath>.

@LB-- LB-- added this to the v1.1.1 milestone Nov 2, 2021
@LB-- LB-- merged commit b9b5d53 into json-parser:master Nov 2, 2021
@DimitriPapadopoulos DimitriPapadopoulos deleted the num_digits branch November 3, 2021 07:56
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 this pull request may close these issues.

None yet

2 participants