[PATCH v4 0/2] MR10961: scrrun/dictionary: Rename keys in place in put_Key.

[Francis De Brabandere (＠francisdb)]

Renaming a key should preserve its enumeration position and its value,
fail with element-not-found when the source key is absent, and fail with
key-already-exists when the target key is taken.

--
  v4: scrrun/dictionary: Rename keys in place in put_Key.
      scrrun/tests: Add tests for dictionary put_Key.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10961

[PATCH v4 1/2] scrrun/tests: Add tests for dictionary put_Key.

[Francis De Brabandere]

From: Francis De Brabandere <[e-mail]>

Renaming a key should preserve its enumeration position and its value,
fail with element-not-found when the source key is absent, and fail with
key-already-exists when the target key is taken.
---
 dlls/scrrun/tests/dictionary.c | 136 +++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c
index 8fc369e7062..23b52a54dd3 100644
--- a/dlls/scrrun/tests/dictionary.c
+++ b/dlls/scrrun/tests/dictionary.c
@@ -1134,6 +1134,141 @@ static void test_Add(void)
     IDictionary_Release(dict);
 }
 
+static void add_str_item(IDictionary *dict, const WCHAR *name, int value)
+{
+    VARIANT key, item;
+    HRESULT hr;
+
+    V_VT(&key) = VT_BSTR;
+    V_BSTR(&key) = SysAllocString(name);
+    V_VT(&item) = VT_I2;
+    V_I2(&item) = value;
+    hr = IDictionary_Add(dict, &key, &item);
+    ok(hr == S_OK, "Add %s: %#lx.\n", wine_dbgstr_w(name), hr);
+    VariantClear(&key);
+}
+
+static void check_key_at(IDictionary *dict, LONG index, const WCHAR *expect, BOOL todo)
+{
+    VARIANT keys, key;
+    HRESULT hr;
+
+    VariantInit(&keys);
+    hr = IDictionary_Keys(dict, &keys);
+    ok(hr == S_OK, "Keys: %#lx.\n", hr);
+
+    VariantInit(&key);
+    hr = SafeArrayGetElement(V_ARRAY(&keys), &index, &key);
+    ok(hr == S_OK, "GetElement %ld: %#lx.\n", index, hr);
+    todo_wine_if(todo) ok(V_VT(&key) == VT_BSTR && !lstrcmpW(V_BSTR(&key), expect),
+            "key at %ld: got %s, expected %s.\n", index, wine_dbgstr_variant(&key), wine_dbgstr_w(expect));
+    VariantClear(&key);
+    VariantClear(&keys);
+}
+
+static BOOL exists_str(IDictionary *dict, const WCHAR *name)
+{
+    VARIANT_BOOL exists = VARIANT_FALSE;
+    VARIANT key;
+    HRESULT hr;
+
+    V_VT(&key) = VT_BSTR;
+    V_BSTR(&key) = SysAllocString(name);
+    hr = IDictionary_Exists(dict, &key, &exists);
+    ok(hr == S_OK, "Exists %s: %#lx.\n", wine_dbgstr_w(name), hr);
+    VariantClear(&key);
+    return exists == VARIANT_TRUE;
+}
+
+static void test_put_Key(void)
+{
+    VARIANT key, newkey, item;
+    IDictionary *dict;
+    BOOL has_y;
+    LONG count;
+    HRESULT hr;
+
+    hr = CoCreateInstance(&CLSID_Dictionary, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
+            &IID_IDictionary, (void**)&dict);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    add_str_item(dict, L"a", 1);
+    add_str_item(dict, L"b", 2);
+    add_str_item(dict, L"c", 3);
+
+    /* Renaming an existing key keeps its value and enumeration position. */
+    V_VT(&key) = VT_BSTR;
+    V_BSTR(&key) = SysAllocString(L"b");
+    V_VT(&newkey) = VT_BSTR;
+    V_BSTR(&newkey) = SysAllocString(L"x");
+    hr = IDictionary_put_Key(dict, &key, &newkey);
+    ok(hr == S_OK, "put_Key: %#lx.\n", hr);
+    VariantClear(&key);
+    VariantClear(&newkey);
+
+    ok(!exists_str(dict, L"b"), "old key 'b' should be gone\n");
+    ok(exists_str(dict, L"x"), "new key 'x' should exist\n");
+
+    V_VT(&key) = VT_BSTR;
+    V_BSTR(&key) = SysAllocString(L"x");
+    VariantInit(&item);
+    hr = IDictionary_get_Item(dict, &key, &item);
+    ok(hr == S_OK, "get_Item: %#lx.\n", hr);
+    ok(V_VT(&item) == VT_I2 && V_I2(&item) == 2, "renamed item: %s.\n", wine_dbgstr_variant(&item));
+    VariantClear(&key);
+    VariantClear(&item);
+
+    hr = IDictionary_get_Count(dict, &count);
+    ok(hr == S_OK, "get_Count: %#lx.\n", hr);
+    ok(count == 3, "Unexpected count %ld.\n", count);
+
+    check_key_at(dict, 0, L"a", FALSE);
+    check_key_at(dict, 1, L"x", TRUE);
+    check_key_at(dict, 2, L"c", TRUE);
+
+    /* Renaming a key that does not exist fails and changes nothing. */
+    V_VT(&key) = VT_BSTR;
+    V_BSTR(&key) = SysAllocString(L"zzz");
+    V_VT(&newkey) = VT_BSTR;
+    V_BSTR(&newkey) = SysAllocString(L"y");
+    hr = IDictionary_put_Key(dict, &key, &newkey);
+    todo_wine ok(hr == CTL_E_ELEMENT_NOT_FOUND, "put_Key of missing key: %#lx.\n", hr);
+    VariantClear(&key);
+    VariantClear(&newkey);
+
+    hr = IDictionary_get_Count(dict, &count);
+    ok(hr == S_OK, "get_Count: %#lx.\n", hr);
+    todo_wine ok(count == 3, "Unexpected count %ld.\n", count);
+    has_y = exists_str(dict, L"y");
+    todo_wine ok(!has_y, "missing-key rename should not create 'y'\n");
+
+    IDictionary_Release(dict);
+
+    /* Renaming onto an existing key fails and leaves the dictionary intact. */
+    hr = CoCreateInstance(&CLSID_Dictionary, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
+            &IID_IDictionary, (void**)&dict);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    add_str_item(dict, L"a", 1);
+    add_str_item(dict, L"b", 2);
+
+    V_VT(&key) = VT_BSTR;
+    V_BSTR(&key) = SysAllocString(L"b");
+    V_VT(&newkey) = VT_BSTR;
+    V_BSTR(&newkey) = SysAllocString(L"a");
+    hr = IDictionary_put_Key(dict, &key, &newkey);
+    ok(hr == CTL_E_KEY_ALREADY_EXISTS, "put_Key onto existing key: %#lx.\n", hr);
+    VariantClear(&key);
+    VariantClear(&newkey);
+
+    ok(exists_str(dict, L"b"), "key 'b' should be intact after failed rename\n");
+    hr = IDictionary_get_Count(dict, &count);
+    ok(hr == S_OK, "get_Count: %#lx.\n", hr);
+    ok(count == 2, "Unexpected count %ld.\n", count);
+
+    IDictionary_Release(dict);
+}
+
 static void test_IEnumVARIANT(void)
 {
     IUnknown *enum1, *enum2;
@@ -1315,6 +1450,7 @@ START_TEST(dictionary)
     test_Item();
     test_Add();
     test_object_key_hashfail();
+    test_put_Key();
     test_IEnumVARIANT();
     test_putref_Item();
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/10961

[PATCH v4 2/2] scrrun/dictionary: Rename keys in place in put_Key.

[Francis De Brabandere]

From: Francis De Brabandere <[e-mail]>

put_Key implemented a rename as an Add of the new key followed by a
Remove of the old one, which moved the renamed key to the end of the
enumeration order and, when the source key was absent, silently created
a new entry. Rename the existing pair in place instead, preserving its
position, and return CTL_E_ELEMENT_NOT_FOUND when the source key does
not exist.
---
 dlls/scrrun/dictionary.c       | 56 ++++++++++++++++++++++------------
 dlls/scrrun/tests/dictionary.c | 10 +++---
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/dlls/scrrun/dictionary.c b/dlls/scrrun/dictionary.c
index 1b618865360..f1bd646e29e 100644
--- a/dlls/scrrun/dictionary.c
+++ b/dlls/scrrun/dictionary.c
@@ -241,10 +241,19 @@ static HRESULT get_keyitem_pair(struct dictionary *dict, VARIANT *key, struct ke
     return S_OK;
 }
 
+static void link_keyitem_to_bucket(struct dictionary *dict, struct keyitem_pair *pair)
+{
+    struct list *head = get_bucket_head(dict, pair->hash);
+
+    if (!head->next)
+        /* this only happens once per bucket */
+        list_init(head);
+    list_add_tail(head, &pair->bucket);
+}
+
 static HRESULT add_keyitem_pair(struct dictionary *dict, VARIANT *key, VARIANT *item)
 {
     struct keyitem_pair *pair;
-    struct list *head;
     VARIANT hash;
     HRESULT hr;
 
@@ -267,13 +276,8 @@ static HRESULT add_keyitem_pair(struct dictionary *dict, VARIANT *key, VARIANT *
     if (FAILED(hr))
         goto failed;
 
-    head = get_bucket_head(dict, pair->hash);
-    if (!head->next)
-        /* this only happens once per bucket */
-        list_init(head);
-
     /* link to bucket list and to full list */
-    list_add_tail(head, &pair->bucket);
+    link_keyitem_to_bucket(dict, pair);
     list_add_tail(&dict->pairs, &pair->entry);
     dict->count++;
     return S_OK;
@@ -705,29 +709,41 @@ static HRESULT WINAPI dictionary_Items(IDictionary *iface, VARIANT *items)
 static HRESULT WINAPI dictionary_put_Key(IDictionary *iface, VARIANT *key, VARIANT *newkey)
 {
     struct dictionary *dictionary = impl_from_IDictionary(iface);
-    struct keyitem_pair *pair;
-    VARIANT empty;
+    struct keyitem_pair *pair, *existing;
+    VARIANT new_key, hash;
     HRESULT hr;
 
     TRACE("%p, %s, %s.\n", iface, debugstr_variant(key), debugstr_variant(newkey));
 
     if (FAILED(hr = get_keyitem_pair(dictionary, key, &pair)))
         return hr;
+    if (!pair)
+        return CTL_E_ELEMENT_NOT_FOUND;
 
-    if (pair)
-    {
-        /* found existing pair for a key, add new pair with new key
-           and old item and remove old pair after that */
+    /* The key is renamed in place, preserving its value and enumeration
+       position; only collisions with a different pair are rejected. */
+    if (FAILED(hr = get_keyitem_pair(dictionary, newkey, &existing)))
+        return hr;
+    if (existing && existing != pair)
+        return CTL_E_KEY_ALREADY_EXISTS;
 
-        hr = IDictionary_Add(iface, newkey, &pair->item);
-        if (FAILED(hr))
-            return hr;
+    hr = IDictionary_get_HashVal(iface, newkey, &hash);
+    if (FAILED(hr))
+        return hr;
 
-        return IDictionary_Remove(iface, key);
-    }
+    VariantInit(&new_key);
+    hr = VariantCopyInd(&new_key, newkey);
+    if (FAILED(hr))
+        return hr;
+
+    VariantClear(&pair->key);
+    pair->key = new_key;
+    pair->hash = V_I4(&hash);
 
-    VariantInit(&empty);
-    return IDictionary_Add(iface, newkey, &empty);
+    list_remove(&pair->bucket);
+    link_keyitem_to_bucket(dictionary, pair);
+
+    return S_OK;
 }
 
 static HRESULT WINAPI dictionary_Keys(IDictionary *iface, VARIANT *keys)
diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c
index 23b52a54dd3..3c052caed0e 100644
--- a/dlls/scrrun/tests/dictionary.c
+++ b/dlls/scrrun/tests/dictionary.c
@@ -1223,8 +1223,8 @@ static void test_put_Key(void)
     ok(count == 3, "Unexpected count %ld.\n", count);
 
     check_key_at(dict, 0, L"a", FALSE);
-    check_key_at(dict, 1, L"x", TRUE);
-    check_key_at(dict, 2, L"c", TRUE);
+    check_key_at(dict, 1, L"x", FALSE);
+    check_key_at(dict, 2, L"c", FALSE);
 
     /* Renaming a key that does not exist fails and changes nothing. */
     V_VT(&key) = VT_BSTR;
@@ -1232,15 +1232,15 @@ static void test_put_Key(void)
     V_VT(&newkey) = VT_BSTR;
     V_BSTR(&newkey) = SysAllocString(L"y");
     hr = IDictionary_put_Key(dict, &key, &newkey);
-    todo_wine ok(hr == CTL_E_ELEMENT_NOT_FOUND, "put_Key of missing key: %#lx.\n", hr);
+    ok(hr == CTL_E_ELEMENT_NOT_FOUND, "put_Key of missing key: %#lx.\n", hr);
     VariantClear(&key);
     VariantClear(&newkey);
 
     hr = IDictionary_get_Count(dict, &count);
     ok(hr == S_OK, "get_Count: %#lx.\n", hr);
-    todo_wine ok(count == 3, "Unexpected count %ld.\n", count);
+    ok(count == 3, "Unexpected count %ld.\n", count);
     has_y = exists_str(dict, L"y");
-    todo_wine ok(!has_y, "missing-key rename should not create 'y'\n");
+    ok(!has_y, "missing-key rename should not create 'y'\n");
 
     IDictionary_Release(dict);
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10961