Skip to content

Conversation

dementive
Copy link
Contributor

Adds the missing hashfuncs for Color, Pair, and Callable. Mainly for the fix in godotengine/godot#107289 but I also needed the Pair hash for module compatibility.

Also updated Pair to be the same as the Pair in Godot.

@dementive dementive requested a review from a team as a code owner October 4, 2025 23:05
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! I compared the changes to the code in Godot, and it looks like it matches to me. I have only the one note below

#include <intrin.h>
#endif

#include "godot_cpp/templates/pair.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should using #include <...> like the others here, and be ordered alphabetically

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 and pushed that.

I do realize it probably makes no difference at all but is there a reason godot-cpp uses #include <...> for including godot-cpp files? Typically #include <...> is only used for system includes and that is how it is used upstream too so it's always seemed odd to me that godot-cpp does it like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure - I don't recall ever having discussed it, but it's used consistently. If we decided we wanted to change this, we should change it everywhere. I think the consistency is more important than which include style we use

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Oct 5, 2025
@dsnopek dsnopek added this to the 4.x milestone Oct 5, 2025
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me :-)

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

Labels

cherrypick:4.4 cherrypick:4.5 enhancement This is an enhancement on the current functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants