[ruby-dev:21059] Re: Rational 公開しました。

青木です。

  In mail "[ruby-dev:21034] Rational 公開しました。"
    Shin-ichiro HARA <[e-mail]> wrote:

> 原です。
> 
> 拡張ライブラリ版 Rational を公開しました。

> C 言語が 20 年ぶりで、多分よろしくない記述があると思います。
> なのに、組み込みを目標としています。（大丈夫か？）よろしく
> ご指導おねがいします。

中身については語れないので表面的なところを見ました。

= コーディングスタイル

標準添付を狙うなら ruby コアにそろえたほうがよいでしょう。

  * インデント 4

  * else の位置

      if (....) {
          ....
      }
      else {
          ....
      }

  * 関数や if 文のあとに無駄なセミコロンが付いている
    (これはコンパイラによっては warning が出ます)

  * #include <ctype.h>

  * gcc -Wall でチェックすべきです。
    suggest parentheses around comparison in operand 
    は無視してもいいと思いますがその他はできるだけ潰しましょう。
    特にこのへん↓
    rational.c:360: warning: label `defalut' defined but not used
    rational.c:673: warning: return-type defaults to `int'

  * これは気分的なもんですけど、extconf.rb は mode 644 のが
    よいかと。

  * depend がないと Windows で困ったような気がします。

= セマンティクス

  * rb_rat_raw() など、オブジェクトを生成するところで

      rat = Data_Make_Struct(rb_cRational, struct RRational,
                             rb_rat_mark, rb_rat_finalizer, r);

    というのがありますが、rb_cRational 直打ちにするのではなく
    引数の klass を使うべきです。でないと

      class MyRational < Rational
      end
      p MyRational.new(1,2).class   #=> Rational

    ということになります。まあほとんどの場合はどうでもいいんですが、
    いずれ文句を言われるのでやっといたほうがいいでしょう。

    また、こうすると rb_cRational をローカル変数にできるはずです。

  * 1.8 では文字列引数のチェックは

        Check_Type(s,T_STRING);
        ptr = STR2CSTR(s);

    ではなく

        StringValue(var);

    を使います。

  * これはよく見てないんですが、
    おそらく taint の伝播が必要です。 (see % refe -e OBJ_INFECT)

  * 拡張ライブラリで定義したオブジェクトはデフォルトでは
    マーシャライズできません。Rational の性格を考えると
    対応したほうがよいでしょう。

-------------------------------------------------------------------
青木峰郎