fix for dict update same key after delete it

[jasonjoo2010]

following calls will be fail:

dict_set(map, "test", "demo");
dict_delete(map, "test");
dict_set(map, "test", "new value"); //will not set it

this pr is submitted to fix it

[doyoubi]

Thanks for reporting this issue. The current design of dict is not for manipulating the key dynamically. It does provide dict_delete though, it's never used. In fact, we can't even set an existing key again to change its value, because it would require checking the existence of the key by comparing key string and we just don't need it currently.
Any different ideas? @maralla

[doyoubi]

I think we have 2 choices here:

1. In dict_index, keep scanning the table when there is hash collision. Then support deletion and modification of existing keys.
2. Completely disable dynamic modification of existing keys

In fact the existing codes don't rely on the changing of existing keys. Are you building something requires it? @jasonjoo2010

[jasonjoo2010]

@doyoubi
yelp, i took corvus as a training project several days ago and found the problem.

considering modification, would it never be used to be a full functional dict in future? e.g. delete or update(index -> delete -> set)

Solution 1 maybe has a poor performance
Solution 2 we should mark it as a half functional dict

the commit currently would just reuse some slot deleted before through adding a simple condition when conflicted

[doyoubi]

Or we just support deletion and modification but leave the dict_index unchanged. And we should add some docs on it, especially for the case that we can't set the keys with same hash (as the current dict_index can't work in this case).

[tevino]

Documenting the half functional dict sounds good.

[jasonjoo2010]

The only special things now is that we should DELETE first when the key already exists in the dict.

So we can document THIS for special.

Or we can add this logic inside dict_set if we feel better.

Wouldn't modification be needed in future?

[maralla]

Current dict implementation is very limited. I think we should have a refactor to make it fully functional.

[maralla]

It's much simple than I thought to make it support setting the same key multiple times:

     while (1) {
         struct bucket *bucket = &dict->buckets[pos];
 
+        // Empty, set bucket
         if (!bucket->setted) {
             set_bucket(bucket, hash, key, data);
             return;
         }
 
         probe_dist = PSL(dict->capacity, bucket->hash, pos);
+
+        // Same key, update the bucket
+        if (probe_dist == dist && strcmp(key, bucket->key) == 0) {
+            set_bucket(bucket, hash, key, data);
+            return;
+        }
+
         if (probe_dist < dist) {
             if (bucket->deleted) {
                 set_bucket(bucket, hash, key, data);

The code is not fully tested.

[jasonjoo2010]

@maralla

I do have considered this kind of fix.

But there maybe a memory management problem if we just update the pointer to a new one simply. Caller could FREE the pointer if necessarily when we force him to do existence checking when updating.

[maralla]

I think the memory problem is up to the caller. If the value is dynamically allocated, dict_index should be called before set the key.

[jasonjoo2010]

@maralla

yes, you are right. it does the responsibility of caller.

btw, maybe the small fix from our training project cost us too much discussion.
we can make some conclusions quickly, i think.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}