From: ko1@...
Date: 2014-12-24T02:31:16+00:00
Subject: [ruby-core:67085] [ruby-trunk - Bug #10623] rb_hash_delete() can return Qundef in 2.2-rc1
Issue #10623 has been updated by Koichi Sasada.
How about it?
rb_hash_delete_entry() for (1).
```diff
Index: ChangeLog
===================================================================
--- ChangeLog (revision 48955)
+++ ChangeLog (working copy)
@@ -1,3 +1,19 @@
+Wed Dec 24 11:24:03 2014 Koichi Sasada <ko1@atdot.net>
+
+ * hash.c (rb_hash_delete): return Qnil if there are no corresponding
+ entry. [Bug #10623]
+
+ * hash.c (rb_hash_delete_entry): try delete and return Qundef if there
+ are no corresponding entry.
+
+ * internal.h: add rb_hash_delete_entry()'s declaration.
+
+ * symbol.c: use rb_hash_delete_entry().
+
+ * thread.c: use rb_hash_delete_entry().
+
+ * ext/-test-/hash/delete.c: use rb_hash_delete_entry().
+
Wed Dec 24 09:35:11 2014 NAKAMURA Usaku <usa@ruby-lang.org>
* ext/fiddle/extconf.rb: remove ffitarget.h generated by configure on
Index: ext/-test-/hash/delete.c
===================================================================
--- ext/-test-/hash/delete.c (revision 48955)
+++ ext/-test-/hash/delete.c (working copy)
@@ -3,7 +3,7 @@
static VALUE
hash_delete(VALUE hash, VALUE key)
{
- VALUE ret = rb_hash_delete(hash, key);
+ VALUE ret = rb_hash_delete_entry(hash, key);
return ret == Qundef ? Qnil : rb_ary_new_from_values(1, &ret);
}
Index: hash.c
===================================================================
--- hash.c (revision 48955)
+++ hash.c (working copy)
@@ -969,23 +969,49 @@ rb_hash_index(VALUE hash, VALUE value)
return rb_hash_key(hash, value);
}
+/*
+ * delete a specified entry a given key.
+ * if there is the corresponding entry, return a value of the entry.
+ * if there is no corresponding entry, return Qundef.
+ */
VALUE
-rb_hash_delete(VALUE hash, VALUE key)
+rb_hash_delete_entry(VALUE hash, VALUE key)
{
st_data_t ktmp = (st_data_t)key, val;
- if (!RHASH(hash)->ntbl)
+ if (!RHASH(hash)->ntbl) {
return Qundef;
- if (RHASH_ITER_LEV(hash) > 0) {
- if (st_delete_safe(RHASH(hash)->ntbl, &ktmp, &val, (st_data_t)Qundef)) {
+ }
+ else if (RHASH_ITER_LEV(hash) > 0 &&
+ (st_delete_safe(RHASH(hash)->ntbl, &ktmp, &val, (st_data_t)Qundef))) {
FL_SET(hash, HASH_DELETED);
return (VALUE)val;
}
- }
- else if (st_delete(RHASH(hash)->ntbl, &ktmp, &val))
+ else if (st_delete(RHASH(hash)->ntbl, &ktmp, &val)) {
return (VALUE)val;
+ }
+ else {
return Qundef;
}
+}
+
+/*
+ * delete a specified entry by a given key.
+ * if there is the corresponding entry, return a value of the entry.
+ * if there is no corresponding entry, return Qnil.
+ */
+VALUE
+rb_hash_delete(VALUE hash, VALUE key)
+{
+ VALUE deleted_value = rb_hash_delete_entry(hash, key);
+
+ if (deleted_value != Qundef) { /* likely pass */
+ return deleted_value;
+ }
+ else {
+ return Qnil;
+ }
+}
/*
* call-seq:
@@ -1011,13 +1037,20 @@ rb_hash_delete_m(VALUE hash, VALUE key)
VALUE val;
rb_hash_modify_check(hash);
- val = rb_hash_delete(hash, key);
- if (val != Qundef) return val;
+ val = rb_hash_delete_entry(hash, key);
+
+ if (val != Qundef) {
+ return val;
+ }
+ else {
if (rb_block_given_p()) {
return rb_yield(key);
}
+ else {
return Qnil;
}
+ }
+}
struct shift_var {
VALUE key;
@@ -1063,7 +1096,7 @@ rb_hash_shift(VALUE hash)
else {
rb_hash_foreach(hash, shift_i_safe, (VALUE)&var);
if (var.key != Qundef) {
- rb_hash_delete(hash, var.key);
+ rb_hash_delete_entry(hash, var.key);
return rb_assoc_new(var.key, var.val);
}
}
Index: internal.h
===================================================================
--- internal.h (revision 48955)
+++ internal.h (working copy)
@@ -1119,6 +1119,9 @@ int rb_bug_reporter_add(void (*func)(FIL
VALUE rb_str_normalize_ospath(const char *ptr, long len);
#endif
+/* hash.c (export) */
+VALUE rb_hash_delete_entry(VALUE hash, VALUE key);
+
/* io.c (export) */
void rb_maygvl_fd_fix_cloexec(int fd);
Index: symbol.c
===================================================================
--- symbol.c (revision 48955)
+++ symbol.c (working copy)
@@ -740,7 +740,7 @@ rb_sym2id(VALUE sym)
RSYMBOL(sym)->id = id |= num;
/* make it permanent object */
set_id_entry(num >>= ID_SCOPE_SHIFT, fstr, sym);
- rb_hash_delete(global_symbols.dsymbol_fstr_hash, fstr);
+ rb_hash_delete_entry(global_symbols.dsymbol_fstr_hash, fstr);
}
}
else {
Index: thread.c
===================================================================
--- thread.c (revision 48955)
+++ thread.c (working copy)
@@ -4806,13 +4806,13 @@ recursive_pop(VALUE list, VALUE obj, VAL
return 0;
}
if (RB_TYPE_P(pair_list, T_HASH)) {
- rb_hash_delete(pair_list, paired_obj);
+ rb_hash_delete_entry(pair_list, paired_obj);
if (!RHASH_EMPTY_P(pair_list)) {
return 1; /* keep hash until is empty */
}
}
}
- rb_hash_delete(list, obj);
+ rb_hash_delete_entry(list, obj);
return 1;
}
```
----------------------------------------
Bug #10623: rb_hash_delete() can return Qundef in 2.2-rc1
https://bugs.ruby-lang.org/issues/10623#change-50599
* Author: Aman Gupta
* Status: Open
* Priority: Normal
* Assignee:
* Category:
* Target version: current: 2.2.0
* ruby -v: ruby 2.2.0dev [x86_64-darwin14]
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN
----------------------------------------
The behavior of rb_hash_delete() has changed from 2.1. Before, it would always return Qnil or VALUE. Now it can also return Qundef, which is breaking the posix-spawn gem's usage:
https://github.com/rtomayko/posix-spawn/blob/master/ext/posix-spawn.c#L242-L258
It also appears RTEST(Qundef) returns true, which causes a segfault in posix-spawn gem here:
https://github.com/rtomayko/posix-spawn/blob/master/ext/posix-spawn.c#L425
I think we should revert back to original behavior before 2.2.0 is released.
--
https://bugs.ruby-lang.org/