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

Fix is_valid_float, Variant parser, Expression parser, script highlighter, and TextServer not handing capital E in scientific notation. #102396

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Feb 4, 2025

Fixes #102363

@bruvzg bruvzg added this to the 4.4 milestone Feb 4, 2025
@bruvzg bruvzg requested review from a team as code owners February 4, 2025 06:03
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems fine.

I wonder if we should also support uppercase 0x and 0b which some of those parsers handle.

@bruvzg
Copy link
Member Author

bruvzg commented Feb 4, 2025

I wonder if we should also support uppercase 0x and 0b which some of those parsers handle.

C standard seems to allow both upper and lower case, so probably yes.

@HolonProduction
Copy link
Member

Would make sense to add a test for this, wouldn't it?

@MewPurPur
Copy link
Contributor

Should the check be added in syntax_highlighter.cpp too?

@bruvzg
Copy link
Member Author

bruvzg commented Feb 4, 2025

Should the check be added in syntax_highlighter.cpp too?

Yes, missed it. Will update in a moment.

@bruvzg bruvzg requested a review from a team as a code owner February 4, 2025 11:22
@bruvzg
Copy link
Member Author

bruvzg commented Feb 4, 2025

Will update in a moment.

Done, also added few tests.

static bool isflt[] = { true, true, true, false, true, true, false, false, false, false, false, false, false, true, true };
static bool isaid[] = { false, false, false, false, false, false, false, false, true, true, false, false, false, false, false };
static bool isuid[] = { false, false, false, false, false, false, false, false, true, true, false, false, true, false, false };
for (unsigned int i = 0; i < sizeof(data) / sizeof(data[0]); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not important, but for the future, you can use std::size(data) instead of sizeof(data) / sizeof(data[0]) :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it C++20 only thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nop, I was wrong it's C++17 (and compiler support is reasonable) so it can be used. Might good cleanup for ~70 currently used cases of sizeof / sizeof.

@Repiteo
Copy link
Contributor

Repiteo commented Feb 5, 2025

Needs a rebase

…ighlighter, and `TextServer` not handing capital E in scientific notation.
@Repiteo Repiteo merged commit 78cad5f into godotengine:master Feb 6, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 6, 2025

Thanks!

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.

Capital "E" in E notation is inconsistently handled
7 participants