From: Yukihiro Matsumoto <matz@...>
Date: 2005-03-02T08:50:20+09:00
Subject: [ruby-dev:25783] Re: 鬼車コードレビュー

まつもと ゆきひろです

In message "Re: [ruby-dev:25779] Re: 鬼車コードレビュー"
    on Tue, 1 Mar 2005 21:03:10 +0900, "K.Kosako" <sndgk393@ybb.ne.jp> writes:

|>   (2) regparse.c:name_add()で登録されたNameEntryがどこでも
|>       freeされていないように思える。e->name, e->back_refsは
|>       i_free_name_entry()でfreeされているようだ。
|> 
|> で、それはそれとしてst_*_strendはregparse.cに(renameして)移
|> 動したいのですが構いませんか？
|
|どうぞ。
|(2)のバグは、そちらの作業が終わった後に
|修正します。

ありがとうございます。以下のような修正を行いました。

  * _strend関連をst.[ch]からregparse.cへ移動
  * st_insert()にメモリリークがあったので修正(キーがすでに登
    録されていた時st_strend_keyがリーク)
  * 実際に使われていない関数は削除
  * st.[ch]からfree_key, clone_keyを削除
  * free_keyが呼ばれていたタイミングで明示的にkeyを削除

他の修正と混じっているので、すぐにコミットできませんが、近い
うちにコミットします。パッチを添付します。(2)のバグも一緒に
直したと思いますので、よかったらチェックしてみてください。

|strend関連の移動の他には、st.[ch]の変更は
|ないのでしょうか？

上記のようにfree_key, clone_keyがなくなりました。

Index: st.c
===================================================================
RCS file: /var/cvs/src/ruby/st.c,v
retrieving revision 1.33
diff -p -u -1 -r1.33 st.c
--- st.c	23 Feb 2005 13:10:45 -0000	1.33
+++ st.c	1 Mar 2005 23:44:32 -0000
@@ -58,4 +58,2 @@ static struct st_hash_type type_numhash 
     numhash,
-    st_nothing_key_free,
-    st_nothing_key_clone
 };
@@ -67,16 +65,2 @@ static struct st_hash_type type_strhash 
     strhash,
-    st_nothing_key_free,
-    st_nothing_key_clone
-};
-
-static int strend_cmp(st_strend_key*, st_strend_key*);
-static int strend_hash(st_strend_key*);
-static int strend_key_free(st_data_t key);
-static st_data_t strend_key_clone(st_data_t x);
-
-static struct st_hash_type type_strend_hash = {
-    strend_cmp,
-    strend_hash,
-    strend_key_free,
-    strend_key_clone
 };
@@ -230,9 +214,2 @@ st_init_strtable_with_size(size)
 
-st_table*
-st_init_strend_table_with_size(size)
-    int size;
-{
-    return st_init_table_with_size(&type_strend_hash, size);
-}
-
 void
@@ -248,3 +225,2 @@ st_free_table(table)
 	    next = ptr->next;
-            table->type->key_free(ptr->key);
 	    free(ptr);
@@ -299,17 +275,2 @@ st_lookup(table, key, value)
 
-int
-st_lookup_strend(table, str_key, end_key, value)
-    st_table *table;
-    const unsigned char* str_key;
-    const unsigned char* end_key;
-    st_data_t *value;
-{
-  st_strend_key key;
-
-  key.s   = (unsigned char* )str_key;
-  key.end = (unsigned char* )end_key;
-
-  return st_lookup(table, (st_data_t )(&key), value);
-}
-
 #define ADD_DIRECT(table, key, value, hash_val, bin_pos)\
@@ -354,18 +315,2 @@ st_insert(table, key, value)
 
-int
-st_insert_strend(table, str_key, end_key, value)
-     st_table *table;
-     const unsigned char* str_key;
-     const unsigned char* end_key;
-     st_data_t value;
-{
-  st_strend_key* key;
-
-  key = alloc(st_strend_key);
-  key->s   = (unsigned char* )str_key;
-  key->end = (unsigned char* )end_key;
-
-  return st_insert(table, (st_data_t )key, value);
-}
-
 void
@@ -383,17 +328,2 @@ st_add_direct(table, key, value)
 
-void
-st_add_direct_strend(table, str_key, end_key, value)
-    st_table *table;
-    const unsigned char* str_key;
-    const unsigned char* end_key;
-    st_data_t value;
-{
-  st_strend_key* key;
-
-  key = alloc(st_strend_key);
-  key->s   = (unsigned char* )str_key;
-  key->end = (unsigned char* )end_key;
-  st_add_direct(table, (st_data_t )key, value);
-}
-
 static void
@@ -457,3 +387,2 @@ st_copy(old_table)
 	    *entry = *ptr;
-            entry->key  = old_table->type->key_clone(ptr->key);
 	    entry->next = new_table->bins[i];
@@ -558,3 +487,3 @@ st_cleanup_safe(table, never)
 
-void
+int
 st_foreach(table, func, arg)
@@ -571,3 +500,3 @@ st_foreach(table, func, arg)
 	for(ptr = table->bins[i]; ptr != 0;) {
-	    retval = (*func)(ptr->key, ptr->record, arg, 0);
+	    retval = (*func)(ptr->key, ptr->record, arg);
 	    switch (retval) {
@@ -582,4 +511,3 @@ st_foreach(table, func, arg)
 		    /* call func with error notice */
-		    retval = (*func)(0, 0, arg, 1);
-		    return;
+		    return 1;
 		}
@@ -591,3 +519,3 @@ st_foreach(table, func, arg)
 	    case ST_STOP:
-		return;
+	        return 0;
 	    case ST_DELETE:
@@ -601,3 +529,2 @@ st_foreach(table, func, arg)
 		ptr = ptr->next;
-                table->type->key_free(tmp->key);
 		free(tmp);
@@ -607,2 +534,3 @@ st_foreach(table, func, arg)
     }
+    return 0;
 }
@@ -660,58 +588,2 @@ numhash(n)
     return n;
-}
-
-extern int
-st_nothing_key_free(st_data_t key) { return 0; }
-
-extern st_data_t
-st_nothing_key_clone(st_data_t x) { return x; } 
-
-static int strend_cmp(st_strend_key* x, st_strend_key* y)
-{
-  unsigned char *p, *q;
-  int c;
-
-  if ((x->end - x->s) != (y->end - y->s))
-    return 1;
-
-  p = x->s;
-  q = y->s;
-  while (p < x->end) {
-    c = (int )*p - (int )*q;
-    if (c != 0) return c;
-
-    p++; q++;
-  }
-
-  return 0;
-}
-
-static int strend_hash(st_strend_key* x)
-{
-  int val;
-  unsigned char *p;
-
-  val = 0;
-  p = x->s;
-  while (p < x->end) {
-    val = val * 997 + (int )*p++;
-  }
-
-  return val + (val >> 5);
-}
-
-static int strend_key_free(st_data_t x)
-{
-  xfree((void* )x);
-  return 0;
-}
-
-static st_data_t strend_key_clone(st_data_t x)
-{
-  st_strend_key* new_key;
-  st_strend_key* key = (st_strend_key* )x;
-
-  new_key = alloc(st_strend_key);
-  *new_key = *key;
-  return (st_data_t )new_key;
 }
Index: st.h
===================================================================
RCS file: /var/cvs/src/ruby/st.h,v
retrieving revision 1.13
diff -p -u -1 -r1.13 st.h
--- st.h	23 Feb 2005 13:10:45 -0000	1.13
+++ st.h	1 Mar 2005 23:44:32 -0000
@@ -16,4 +16,2 @@ struct st_hash_type {
     int (*hash)();
-    int (*key_free)();
-    st_data_t (*key_clone)();
 };
@@ -27,7 +25,2 @@ struct st_table {
 
-typedef struct {
-  unsigned char* s;
-  unsigned char* end;
-} st_strend_key;
-
 #define st_is_member(table,key) st_lookup(table,key,(st_data_t *)0)
@@ -53,3 +46,2 @@ st_table *st_init_strtable _((void));
 st_table *st_init_strtable_with_size _((int));
-st_table *st_init_strend_table_with_size _((int));
 int st_delete _((st_table *, st_data_t *, st_data_t *));
@@ -57,8 +49,5 @@ int st_delete_safe _((st_table *, st_dat
 int st_insert _((st_table *, st_data_t, st_data_t));
-int st_insert_strend _((st_table *, const unsigned char*, const unsigned char*, st_data_t));
 int st_lookup _((st_table *, st_data_t, st_data_t *));
-int st_lookup_strend _((st_table *, const unsigned char*, const unsigned char*, st_data_t*));
-void st_foreach _((st_table *, int (*)(ANYARGS), st_data_t));
+int st_foreach _((st_table *, int (*)(ANYARGS), st_data_t));
 void st_add_direct _((st_table *, st_data_t, st_data_t));
-void st_add_direct_strend _((st_table *, const unsigned char*, const unsigned char*, st_data_t));
 void st_free_table _((st_table *));
@@ -66,5 +55,2 @@ void st_cleanup_safe _((st_table *, st_d
 st_table *st_copy _((st_table *));
-
-extern st_data_t st_nothing_key_clone _((st_data_t key));
-extern int st_nothing_key_free _((st_data_t key));
 
Index: regparse.c
===================================================================
RCS file: /var/cvs/src/ruby/regparse.c,v
retrieving revision 1.15
diff -p -u -1 -r1.15 regparse.c
--- regparse.c	23 Feb 2005 12:47:23 -0000	1.15
+++ regparse.c	1 Mar 2005 23:44:33 -0000
@@ -307,2 +307,84 @@ typedef struct {
 
+typedef struct {
+  unsigned char* s;
+  unsigned char* end;
+} st_strend_key;
+
+static int strend_cmp(st_strend_key*, st_strend_key*);
+static int strend_hash(st_strend_key*);
+
+static struct st_hash_type type_strend_hash = {
+  strend_cmp,
+  strend_hash,
+};
+
+static st_table*
+onig_st_init_strend_table_with_size(int size)
+{
+    return onig_st_init_table_with_size(&type_strend_hash, size);
+}
+
+static int
+onig_st_lookup_strend(st_table *table, const UChar* str_key, const UChar* end_key, st_data_t *value)
+{
+    st_strend_key key;
+
+    key.s   = (unsigned char* )str_key;
+    key.end = (unsigned char* )end_key;
+
+    return onig_st_lookup(table, (st_data_t )(&key), value);
+}
+
+static int
+onig_st_insert_strend(st_table *table, const UChar* str_key, const UChar* end_key, st_data_t value)
+{
+  st_strend_key* key;
+  int result;
+
+  key = (st_strend_key* )xmalloc(sizeof(st_strend_key));
+  key->s   = (unsigned char* )str_key;
+  key->end = (unsigned char* )end_key;
+  result = onig_st_insert(table, (st_data_t )key, value);
+  if (result) {
+    xfree(key);
+  }
+  return result;
+}
+
+static int
+strend_cmp(st_strend_key* x, st_strend_key* y)
+{
+  unsigned char *p, *q;
+  int c;
+
+  if ((x->end - x->s) != (y->end - y->s))
+    return 1;
+
+  p = x->s;
+  q = y->s;
+  while (p < x->end) {
+    c = (int )*p - (int )*q;
+    if (c != 0) return c;
+
+    p++; q++;
+  }
+
+  return 0;
+}
+
+static int
+strend_hash(st_strend_key* x)
+{
+  int val;
+  unsigned char *p;
+
+  val = 0;
+  p = x->s;
+  while (p < x->end) {
+    val = val * 997 + (int )*p++;
+  }
+
+  return val + (val >> 5);
+}
+
 typedef st_table  NameTable;
@@ -352,4 +434,6 @@ i_free_name_entry(UChar* key, NameEntry*
 {
-  xfree(e->name);  /* == key */
+  xfree(e->name);
   if (IS_NOT_NULL(e->back_refs)) xfree(e->back_refs);
+  xfree(key);
+  xfree(e);
   return ST_DELETE;
@@ -4548,18 +4632,2 @@ static int type_cclass_hash(type_cclass_
 
-static int type_cclass_key_free(st_data_t x)
-{
-  xfree((void* )x);
-  return 0;
-}
-
-static st_data_t type_cclass_key_clone(st_data_t x)
-{
-  type_cclass_key* new_key;
-  type_cclass_key* key = (type_cclass_key* )x;
-
-  new_key = (type_cclass_key* )xmalloc(sizeof(type_cclass_key));
-  *new_key = *key;
-  return (st_data_t )new_key;
-}
-
 static struct st_hash_type type_type_cclass_hash = {
@@ -4567,4 +4635,2 @@ static struct st_hash_type type_type_ccl
     type_cclass_hash,
-    type_cclass_key_free,
-    type_cclass_key_clone
 };
Index: hash.c
===================================================================
RCS file: /var/cvs/src/ruby/hash.c,v
retrieving revision 1.146
diff -p -u -1 -r1.146 hash.c
--- hash.c	28 Jan 2005 15:21:48 -0000	1.146
+++ hash.c	1 Mar 2005 23:44:33 -0000
@@ -104,4 +104,2 @@ static struct st_hash_type objhash = {
     rb_any_hash,
-    st_nothing_key_free,
-    st_nothing_key_clone
 };
@@ -115,3 +113,3 @@ struct foreach_safe_arg {
 static int
-foreach_safe_i(key, value, arg, err)
+foreach_safe_i(key, value, arg)
     st_data_t key, value;
@@ -121,7 +119,4 @@ foreach_safe_i(key, value, arg, err)
 
-    if (err) {
-	rb_raise(rb_eRuntimeError, "hash modified during iteration");
-    }
     if (key == Qundef) return ST_CONTINUE;
-    status = (*arg->func)(key, value, arg->arg, err);
+    status = (*arg->func)(key, value, arg->arg);
     if (status == ST_CONTINUE) {
@@ -143,3 +138,5 @@ st_foreach_safe(table, func, a)
     arg.arg = a;
-    st_foreach(table, foreach_safe_i, (st_data_t)&arg);
+    if (st_foreach(table, foreach_safe_i, (st_data_t)&arg)) {
+	rb_raise(rb_eRuntimeError, "hash modified during iteration");
+    }
 }
@@ -153,6 +150,5 @@ struct hash_foreach_arg {
 static int
-hash_foreach_iter(key, value, arg, err)
+hash_foreach_iter(key, value, arg)
     VALUE key, value;
     struct hash_foreach_arg *arg;
-    int err;
 {
@@ -161,5 +157,2 @@ hash_foreach_iter(key, value, arg, err)
 
-    if (err) {
-	rb_raise(rb_eRuntimeError, "hash modified during iteration");
-    }
     tbl = RHASH(arg->hash)->tbl;
@@ -201,3 +194,5 @@ hash_foreach_call(arg)
 {
-    st_foreach(RHASH(arg->hash)->tbl, hash_foreach_iter, (st_data_t)arg);
+    if (st_foreach(RHASH(arg->hash)->tbl, hash_foreach_iter, (st_data_t)arg)) {
+ 	rb_raise(rb_eRuntimeError, "hash modified during iteration");
+    }
     return Qnil;