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

StringName comparisons give wrong and inconsistent results #76218

Closed
lilizoey opened this issue Apr 18, 2023 · 8 comments · Fixed by #77197
Closed

StringName comparisons give wrong and inconsistent results #76218

lilizoey opened this issue Apr 18, 2023 · 8 comments · Fixed by #77197
Milestone

Comments

@lilizoey
Copy link

Godot version

4.0.2

System information

Linux 5.10.167-2-MANJARO

Issue description

Comparing StringNames will give wrong and inconsistent results, running the provided code will give the results:

a < b false
a > b false
a <= b true
a >= b true
a == b false

The documentation for < on StringName says:

Returns true if the left String comes before right in Unicode order, which roughly matches the alphabetical order. Useful for sorting.

However then "a < b" should be true, not false.

In addition a <= b and b <= a is true, yet a == b is false. Which is inconsistent.

Steps to reproduce

run

func _ready():
	var a = &"a"
	var b = &"b"
	print("a < b ", a < b)
	print("a > b ", a > b)
	print("a <= b ", a <= b)
	print("a >= b ", a >= b)
	print("a == b ", a == b)

Minimal reproduction project

N/A

@AThousandShips
Copy link
Member

The documentation is apparently wrong from the implementation, should be updated

@KoBeWi
Copy link
Member

KoBeWi commented Apr 18, 2023

The operator is implemented like this

_FORCE_INLINE_ bool operator<(const StringName &p_name) const {
return _data < p_name._data;
}

_data is a pointer. It's wrongly implemented (unless there is a reason why it's like that).

@AThousandShips
Copy link
Member

AThousandShips commented Apr 18, 2023

Internally StringNames are compared by memory to keep it fast it seems, when stored in RBMap, the fact that the documentation talks about String and not StringName would imply to me that it was copied from String without considering the actual implementation

@AThousandShips
Copy link
Member

I assume StringName was/are used in RBMap etc, sorting the pointers should give uniqueness and is faster than comparing by the content, seems to be overlooked

@AThousandShips
Copy link
Member

In fact the wording for String and StringName are equal for the ordering operators, including calling it String in StringName, however the equality operator differs, and makes a note about how it is faster:

Returns true if the StringName and right refer to the same name. Comparisons between StringNames are much faster than regular String comparisons.

@AThousandShips
Copy link
Member

AThousandShips commented Apr 18, 2023

The implementation is weird though as it doesn't give correct results, also there isn't any other comparison operators for StringName than < so might be doing conversion under the hood somewhere

@AThousandShips
Copy link
Member

For now if you want the alphabetic ordering a workaround should be to cast them to String

I suspect there's some conversion going on as there's no errors caused by the fact that StringName doesn't have any of the remaining operators, and a decision would have to be made on what ordering to use, alphabetical or pointer based

@AThousandShips
Copy link
Member

Working on a fix for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants