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

Accurately document StringName comparisons #77083

Merged
merged 1 commit into from
May 18, 2023
Merged

Accurately document StringName comparisons #77083

merged 1 commit into from
May 18, 2023

Conversation

Jess3Jane
Copy link
Contributor

@Jess3Jane Jess3Jane commented May 15, 2023 •
edited
Loading

A few months ago someone kindly wrote documentation for the operators of both String and StringName (#68217). Unfortunately, StringName's comparison operators do not function the same as the ones for String do, comparing only the order of the pointers (at least, if my experimentation and understanding of the engine code are to be believed).

This PR simply corrects the documentation of the comparison operators for StringName.

@Jess3Jane Jess3Jane requested a review from a team as a code owner May 15, 2023 01:20
MewPurPur
MewPurPur reviewed May 15, 2023
View reviewed changes
@akien-mga akien-mga added this to the 4.1 milestone May 15, 2023
@mhilbrunner
Copy link
Member

Please squash the commits into one :)

@AThousandShips
Copy link
Member

AThousandShips commented May 18, 2023 •
edited
Loading

For comparison other than ==, !=, and < this isn't necessarily correct, comparison seems to be broken for StringName, see #76218, currently working on a fix for that

Fix in #77197, required for this to be fully correct in documentation

It could also be useful to add a note about casting to String to get unicode order as it mentions it does not use that

RandomShaper
RandomShaper approved these changes May 18, 2023
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Should be merged with #77197.

@RandomShaper RandomShaper mentioned this pull request May 18, 2023
Merged
@YuriSizov YuriSizov merged commit 2dc3294 into godotengine:master May 18, 2023
@YuriSizov
Copy link
Contributor

Thanks, and congrats on your first merged Godot PR!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MewPurPur MewPurPur MewPurPur left review comments

@YuriSizov YuriSizov YuriSizov approved these changes

@RandomShaper RandomShaper RandomShaper approved these changes

Assignees
No one assigned
Labels
documentation enhancement
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
7 participants
@Jess3Jane @mhilbrunner @AThousandShips @YuriSizov @RandomShaper @MewPurPur @akien-mga