コードのレビューをもらうためには

このエントリの続き。

昨日良い例見つけたから、それを例にして書く。OSS関係なくて、ソフトウェアの開発でよくあること。この例で出てきてる方、例に使っちゃってすみません。

Saitoさんって方が以前このバグに対してのパッチを書いていたようなんだけど、作ったパッチをことごとくリジェクトしてるBorisの言い分は簡単で、

• 説明足りない (何故説明が必要かは以下に含まれる)
• おまえ、直す必要のないところ弄っているけど、なんでこのバグでいじってるの?
• 文字列作り直してからParseEnumValue読んでるけど、なんで先に正規化する必要があるのか、理解できん
• こんなところで文字列作り変えることするんだったら、そもそもString APIがクソなんだから機能足そうよ

ってだけなんだよね、簡単に言えば。

Saitoさんは、ParseEnumValueを直接呼ばなくて文字列を組み立て直してから、ParseEnumValueを呼ぶようにしてる。そうすべきだって主張してるけど、個人的に見れば、パフォーマンス的にどうかと思う (エラーのリトライでやるんだったらわかるけど) のと、ここで文字列組み立て直す必要あんの？もっとEasyにやろうよって思うんだよね。数字以外の文字で切り捨てて、もう一度ParseEnumValueを呼ぶようにするとかさ (ついでにそういうシンプルなパッチ書いたけど)。

しかも文字列を組み直すところのやり方は非常にダメ。Borisも非常にダメだししてるけど、これはあり得んというか、String APIが非常に用意されているので、それを使えばいいのに、ANSI文字列を作ってそれをUNICODEに変換なんて発想がひどすぎ。すくなくとも直接UNICODEの文字列作ればいいのにさ。ちなみに、Mozilla的な感じだと、nsString::AppendIntとかnsString::Assignを駆使するのがベターで、ちょー簡単。

どのOSでもアプリでもOSSじゃなくてもプロプラでもそうだけど、大人数で作るプログラムというのは、豊富なライブラリも用意されてて、それを使うようにコーディングルールが作られている。Apacheだって、APR (Apache Portable Runtime)をできる限り使うとか決まっているし、Googleのコードだって、ルール通りに書かないといけないわけだ (そうじゃなければチェックインできないよ)。

日本人にとってはBorisの言い方はキツいのかもしれないけど、まだちゃんと細かく書いてくれるだけ、Boris優しいって思うよ。彼の言うように直せばたぶんSaitoさんの貢献がバグ修正という形になったと思うんだけど、そこでイヤになったか忙しくなったか、別の理由とかでそれ以上はパッチを書かなかったってのが、現在の状態ってところ。

また、彼は別のところを直そうともしてるんだけど、このバグで一緒にやるから面倒なことになる。それをやるんだったら、新しいバグ立てるべきなんだ。

こういう風にバグを分けるべきというのかOSS特有のことじゃなくて、プロプラな会社（しかも外資とか）内でのバグ報告とか修正の話だったら、当然すべきこと (そういう会社に居たときに、こういうこと分からない人が結構居てて、そういう人には説教してた)。アメリカ人というか欧米人だったらそうやるのが普通だから、それに従ってやるべきなんだ。

こんなこと書いてるけど、最後に言いたいのは、<font size="-0.5">とかで変なことになるGeckoにはウケたけど、同様な問題 (-0.5とか-2.5とか+2.5とかを指定) がWebKitにも存在してたこと。マジウケる。