[ruby-dev:25783] Re: 鬼車コードレビュー
From:
Yukihiro Matsumoto <matz@...>
Date:
2005-03-01 23:50:20 UTC
List:
ruby-dev #25783
まつもと ゆきひろです
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;