From 79f654ced5525515091c99a8d23dccb9c2a09b35 Mon Sep 17 00:00:00 2001 From: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:56:54 +0200 Subject: [PATCH 1/2] [Core] Fix sorting of `Dictionary` keys `StringName` keys were sorted as `StringName` which is unstable. --- core/io/json.cpp | 2 +- core/variant/variant.h | 13 +++++++++++++ core/variant/variant_parser.cpp | 2 +- editor/plugins/visual_shader_editor_plugin.cpp | 9 ++++----- modules/gdscript/editor/gdscript_docgen.cpp | 2 +- 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/core/io/json.cpp b/core/io/json.cpp index 664ff7857b6..22219fca295 100644 --- a/core/io/json.cpp +++ b/core/io/json.cpp @@ -121,7 +121,7 @@ String JSON::_stringify(const Variant &p_var, const String &p_indent, int p_cur_ d.get_key_list(&keys); if (p_sort_keys) { - keys.sort(); + keys.sort_custom(); } bool first_key = true; diff --git a/core/variant/variant.h b/core/variant/variant.h index c76b849abdb..3b1924e8eab 100644 --- a/core/variant/variant.h +++ b/core/variant/variant.h @@ -854,6 +854,19 @@ struct StringLikeVariantComparator { static bool compare(const Variant &p_lhs, const Variant &p_rhs); }; +struct StringLikeVariantOrder { + static _ALWAYS_INLINE_ bool compare(const Variant &p_lhs, const Variant &p_rhs) { + if (p_lhs.is_string() && p_rhs.is_string()) { + return p_lhs.operator String() < p_rhs.operator String(); + } + return p_lhs < p_rhs; + } + + _ALWAYS_INLINE_ bool operator()(const Variant &p_lhs, const Variant &p_rhs) const { + return compare(p_lhs, p_rhs); + } +}; + Variant::ObjData &Variant::_get_obj() { return *reinterpret_cast(&_data._mem[0]); } diff --git a/core/variant/variant_parser.cpp b/core/variant/variant_parser.cpp index f5f96456d32..f05b9cd83ab 100644 --- a/core/variant/variant_parser.cpp +++ b/core/variant/variant_parser.cpp @@ -2245,7 +2245,7 @@ Error VariantWriter::write(const Variant &p_variant, StoreStringFunc p_store_str } else { List keys; dict.get_key_list(&keys); - keys.sort(); + keys.sort_custom(); if (keys.is_empty()) { // Avoid unnecessary line break. diff --git a/editor/plugins/visual_shader_editor_plugin.cpp b/editor/plugins/visual_shader_editor_plugin.cpp index ede8351e413..a5df9edcf02 100644 --- a/editor/plugins/visual_shader_editor_plugin.cpp +++ b/editor/plugins/visual_shader_editor_plugin.cpp @@ -2128,12 +2128,11 @@ void VisualShaderEditor::_update_nodes() { } } - Array keys = added.keys(); - keys.sort(); - - for (int i = 0; i < keys.size(); i++) { - const Variant &key = keys.get(i); + List keys; + added.get_key_list(&keys); + keys.sort_custom(); + for (const Variant &key : keys) { const Dictionary &value = (Dictionary)added[key]; add_custom_type(value["name"], value["type"], value["script"], value["description"], value["return_icon_type"], value["category"], value["highend"]); diff --git a/modules/gdscript/editor/gdscript_docgen.cpp b/modules/gdscript/editor/gdscript_docgen.cpp index 32ef429b0d4..758887a7236 100644 --- a/modules/gdscript/editor/gdscript_docgen.cpp +++ b/modules/gdscript/editor/gdscript_docgen.cpp @@ -217,7 +217,7 @@ String GDScriptDocGen::_docvalue_from_variant(const Variant &p_variant, int p_re List keys; dict.get_key_list(&keys); - keys.sort(); + keys.sort_custom(); for (List::Element *E = keys.front(); E; E = E->next()) { if (E->prev()) { From 610635e1c8d8940397723052088979d16aa30a40 Mon Sep 17 00:00:00 2001 From: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com> Date: Fri, 27 Sep 2024 17:43:40 +0200 Subject: [PATCH 2/2] Add test --- tests/core/variant/test_variant.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/core/variant/test_variant.h b/tests/core/variant/test_variant.h index be615975f88..599a282b200 100644 --- a/tests/core/variant/test_variant.h +++ b/tests/core/variant/test_variant.h @@ -1806,6 +1806,14 @@ TEST_CASE("[Variant] Writer and parser dictionary") { CHECK_MESSAGE(d_parsed == Variant(d), "Should parse back."); } +TEST_CASE("[Variant] Writer key sorting") { + Dictionary d = build_dictionary(StringName("C"), 3, "A", 1, StringName("B"), 2, "D", 4); + String d_str; + VariantWriter::write_to_string(d, d_str); + + CHECK_EQ(d_str, "{\n\"A\": 1,\n&\"B\": 2,\n&\"C\": 3,\n\"D\": 4\n}"); +} + TEST_CASE("[Variant] Writer recursive dictionary") { // There is no way to accurately represent a recursive dictionary, // the only thing we can do is make sure the writer doesn't blow up