From 9a146337e8e76d5ec120a2c1ba48da06647f1b26 Mon Sep 17 00:00:00 2001 From: Garrison Jensen Date: Tue, 9 Jan 2024 09:59:59 -0800 Subject: [PATCH 1/3] Check if key exists before deleting it --- ext/containers/rbtree_map/rbtree.c | 4 ++++ lib/containers/rb_tree_map.rb | 2 +- spec/rb_tree_map_spec.rb | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/ext/containers/rbtree_map/rbtree.c b/ext/containers/rbtree_map/rbtree.c index 24cf1e4d..c4bd4f8e 100644 --- a/ext/containers/rbtree_map/rbtree.c +++ b/ext/containers/rbtree_map/rbtree.c @@ -414,6 +414,10 @@ static VALUE rbtree_delete(VALUE self, VALUE key) { rbtree *tree = get_tree_from_self(self); if(!tree->root) return Qnil; + + if(get(tree, tree->root, key) == Qnil) { + return Qnil; + } tree->root = delete(tree, tree->root, key, &deleted_value); if(tree->root) diff --git a/lib/containers/rb_tree_map.rb b/lib/containers/rb_tree_map.rb index c7bf63c8..9386bc21 100644 --- a/lib/containers/rb_tree_map.rb +++ b/lib/containers/rb_tree_map.rb @@ -129,7 +129,7 @@ def max_key # map.min_key #=> "GA" def delete(key) result = nil - if @root + if @root && get(key) @root, result = delete_recursive(@root, key) @root.color = :black if @root end diff --git a/spec/rb_tree_map_spec.rb b/spec/rb_tree_map_spec.rb index 32040aaa..60c9e141 100644 --- a/spec/rb_tree_map_spec.rb +++ b/spec/rb_tree_map_spec.rb @@ -88,6 +88,10 @@ counter += 1 end end + + it "should allow deleting non-existent keys" do + expect(@tree.delete(100000)).to be_nil + end end describe "empty rbtreemap" do From 955f7766d999f7326d197fe12c9463a27df0cfb2 Mon Sep 17 00:00:00 2001 From: Garrison Jensen Date: Tue, 9 Jan 2024 13:33:40 -0800 Subject: [PATCH 2/3] Remove superfluous nil check --- ext/containers/rbtree_map/rbtree.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/ext/containers/rbtree_map/rbtree.c b/ext/containers/rbtree_map/rbtree.c index c4bd4f8e..bd609a86 100644 --- a/ext/containers/rbtree_map/rbtree.c +++ b/ext/containers/rbtree_map/rbtree.c @@ -412,9 +412,6 @@ static VALUE rbtree_max_key(VALUE self) { static VALUE rbtree_delete(VALUE self, VALUE key) { VALUE deleted_value; rbtree *tree = get_tree_from_self(self); - if(!tree->root) - return Qnil; - if(get(tree, tree->root, key) == Qnil) { return Qnil; } From 127303ec71524c53e94c6eaaa63c571343bc7d72 Mon Sep 17 00:00:00 2001 From: Garrison Jensen Date: Tue, 9 Jan 2024 15:47:55 -0800 Subject: [PATCH 3/3] Refactor out non-existing element in spec test --- spec/rb_tree_map_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/rb_tree_map_spec.rb b/spec/rb_tree_map_spec.rb index 60c9e141..63ba6990 100644 --- a/spec/rb_tree_map_spec.rb +++ b/spec/rb_tree_map_spec.rb @@ -34,6 +34,7 @@ shared_examples "non-empty rbtree" do before(:each) do @num_items = 1000 + @non_existing_element = @num_items+1 @random_array = Array.new(@num_items) { rand(@num_items) } @random_array.each { |x| @tree[x] = x } end @@ -51,7 +52,7 @@ end it "should not #has_key? keys it doesn't have" do - expect(@tree.has_key?(100000)).to be false + expect(@tree.has_key?(@non_existing_element)).to be false end it "should #has_key? keys it does have" do @@ -90,7 +91,7 @@ end it "should allow deleting non-existent keys" do - expect(@tree.delete(100000)).to be_nil + expect(@tree.delete(@non_existing_element)).to be_nil end end