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

Add more C++ unit tests for String to number conversions #85666

Conversation

miv391
Copy link
Contributor

@miv391 miv391 commented Dec 2, 2023

This PR adds more C++ units tests to the following string to number conversion functions:

  • String::to_int()
  • String::hex_to_int()
  • String::bin_to_int()
  • String::to_float()

There are inconsistencies with the conversion functions, but the goal of this PR is not to fix anything, the goal is to document how the conversions currently work using the unit tests.

I'm planning to implement this godotengine/godot-proposals#8546, but I'm not touching conversion functions (even a little bit) without first adding more unit tests.

@miv391 miv391 requested a review from a team as a code owner December 2, 2023 16:19
@AThousandShips AThousandShips added this to the 4.x milestone Dec 2, 2023
static const char *nums[4] = { "1237461283", "- 22", "0", " - 1123412" };
static const int num[4] = { 1237461283, -22, 0, -1123412 };
static const char *nums[13] = { "1237461283", "- 22", "0", " - 1123412", "", "10_000_000", "-1_2_3_4", "10__000", " 1 2 34 ", "-0", "007", "0b1011", "0x1012" };
static const int num[13] = { 1237461283, -22, 0, -1123412, 0, 10000000, -1234, 10000, 1234, 0, 7, 1011, 1012 };
Copy link
Member

@AThousandShips AThousandShips Dec 2, 2023

Choose a reason for hiding this comment

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

Invalid suggestion
These values are not aligned, 1011 in decimal is not the same as 1011 in binary, same for hexadecimal 1012 and decimal 1012

Copy link
Contributor Author

@miv391 miv391 Dec 2, 2023

Choose a reason for hiding this comment

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

"0b1011" and "0x1012" just look like a binary and a hexadecimal number, but to_int() doesn't handle them as such. to_int() ignores any non digits, so what it sees is "01011" and "01012" and then converts them as base-10, to 1011 and 1012.

Copy link
Member

Choose a reason for hiding this comment

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

My bad, good point, that should be noted I think in the tests to clarify it's correct behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added comments for those cases. I also made some other changes as I found some new cases which hex_to_int() and bin_to_int() can't handle. For to_int() there just don't seem to be any other invalid cases than too small/large integers as all non-digits (except leading minuses) are ignored.

@miv391 miv391 force-pushed the more-unit-tests-for-string-to-number-conversions branch from b92229a to d782c1b Compare December 2, 2023 16:58
// Invalid float strings should return 0.
static const char *invalid_nums[6] = { "qwerty", "qwerty123", "0xffff", "0b1010", "--3.13", "__345" };

ERR_PRINT_OFF
Copy link
Member

@AThousandShips AThousandShips Dec 2, 2023

Choose a reason for hiding this comment

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

(my bad was meant here)
Is this really required? From what I can see there's no errors on this thrown (only a warning with too high exponent, which you do not test for)

Copy link
Contributor Author

@miv391 miv391 Dec 2, 2023

Choose a reason for hiding this comment

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

You are correct, only hex_to_int() prints errors when the string contains illegal characters and 0 is returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the unnecessary ERR_PRINT_OFF's are removed and "too high exponent" tests are added.

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

The too high exponent case isn't triggered here as it fires with exponent of 512, unsure why it's still infinity here though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this. There seems to be different levels of "too high exponents". Floats between 1e309 - 1e511 will become INFINITY, but warning is not printed (hex_to_int prints errors, btw). When the float gets even larger, 1e512, warning is finally printed. Don't ask me why floats work like this :)

@miv391 miv391 force-pushed the more-unit-tests-for-string-to-number-conversions branch 2 times, most recently from dda7c2a to 4925025 Compare December 2, 2023 17:24
@miv391 miv391 force-pushed the more-unit-tests-for-string-to-number-conversions branch 2 times, most recently from 4f5e035 to 4a8534d Compare December 2, 2023 18:44
}

// Very large exponents.
CHECK(String("1e308").to_float() == 100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CHECK(String("1e308").to_float() == 100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.0);
CHECK(String("1e308").to_float() == 1e308);

Unless there's some reason that won't work, or it will be very cluttered and error prone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@miv391 miv391 force-pushed the more-unit-tests-for-string-to-number-conversions branch from 4a8534d to 5e40124 Compare December 2, 2023 18:54
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code looks good on my end. Thanks for adding more tests 🙂

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Dec 7, 2023
@akien-mga akien-mga added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Dec 7, 2023
@YuriSizov YuriSizov merged commit 47cd07a into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged contribution to the engine!

@miv391 miv391 deleted the more-unit-tests-for-string-to-number-conversions branch December 10, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants