遠藤です。 API レベルで見える追加のある、一見するとでかいコミットがいきなり入った ので、びっくりしました。一応 ML での予告が欲しいです。 それはともかく、ざっとコードレビューの真似事をしてみました。 ○ LIKELY / UNLIKELY の名前がまずい (重大) RB_LIKELY など、他のライブラリと衝突しなさそうな名前にする必要があります。 また、ruby.h に書かれるということは、LIKELY / UNLIKELY は公開 API なの でしょうか (個人的には、ruby.h に書かれているだけでは公開 API ではないと 思っていますが、Yugui さんの考えではそうではなさそうなので) 。 とりあえず /** @internal */ とかつけるとどうでしょう。 ○ RUBY_EVENT_RESCUE が追加された (やや重大) これは dtrace に関係なく set_trace_func でも見える API レベルの変更です。 $ ./ruby -e ' set_trace_func(proc {|name, | p name }) begin raise rescue end ' "c-return" "line" *snip* "c-return" "unknown" # <== この行!! 3 点あります。 - API レベルの仕様変更だけど OK ? 今回のコミットとは別に議論すべき ではないか - unknown という名前は直すべき - これで得られる binding は使っても大丈夫? (SEGV とかしない?) ○ UNLIKELY が使われているところと使われていないところがある gc.c の中に if (UNLIKELY(TRACE_RAISE_ENABLED())) FIRE_RAISE_FATAL(); という箇所と、 if (TRACE_OBJECT_FREE_ENABLED()) FIRE_OBJECT_FREE(rb_obj_id(obj)); という箇所があります。この違いは意図的でしょうか。それとも単なる付け 忘れ? 他にもいくつか UNLIKELY がついていない probe がありました。 ○ rb_class2name_without_alloc はなぜ必要か variable.c に rb_class2name_without_alloc という関数が追加されています が、grep しただけではどこで使われているのかわかりませんでした。 これは今回の変更で一緒に追加する必要があったのでしょうか。 ○ thread_pthread.c に probe を入れるのは OK ? thread_pthread.c にいろいろ入れるのはやや気持ち悪いです。 ささださんが OK と言えば OK だと思いますが。 異常終了の exit の直前に FIRE_RAISE_FATAL() を入れたかっただけのように 見えますが、似た文脈で exit を読んでいる箇所は regcomp.c などにもちらほら あるようです。 ruby_fatal_exit とか作って、従来異常終了の文脈で exit を読んでいる箇所 では全部 ruby_fatal_exit を呼ぶようにしたらどうでしょう。 ○ probe 拡張ライブラリが荒削り usa さんの突っ込みに同意です。 [ruby-dev:39958] それにしても、gcc に AOP があればなあと思うような変更ですね。 -- Yusuke ENDOH <mame / tsg.ne.jp>