Project

General

Profile

Actions

Backport #5702

closed

backport r33935, r33987 (fix private constant)

Added by mame (Yusuke Endoh) over 12 years ago. Updated about 12 years ago.

Status:
Closed
Assignee:
-
[ruby-dev:44925]

Description

r33935 を ruby_1_9_3 にバックポート希望です。
([ruby-list:48558] のバグ修正)
誰かレビュー頼みます。

--
Yusuke Endoh


Files

backport_5702.patch (950 Bytes) backport_5702.patch nagachika (Tomoyuki Chikanaga), 12/05/2011 10:14 AM
bug5702.patch (4.14 KB) bug5702.patch nagachika (Tomoyuki Chikanaga), 02/11/2012 03:24 PM

Updated by kosaki (Motohiro KOSAKI) over 12 years ago


Backport #5702: backport r33935
http://redmine.ruby-lang.org/issues/5702

Author: Yusuke Endoh
Status: Open
Priority: Normal
Assignee: Yusuke Endoh
Category:
Target version:

r33886 を ruby_1_9_3 にバックポート希望です。
([ruby-list:48558] のバグ修正)
誰かレビュー頼みます。

一瞬レビューしようと思ったけど、これは無理。あきらめた。
コアなコミッタの降臨を期待します

ところでバックポートチケットに修正内容書かずにレビューワーほいほい
しようとするライフハックはどこ由来なんでしょう。正直僕はあんまり
うれしくないなあ・・・

あからさまに関係ないのみいちいちコミットを探して関係無いことを

確認しないといけないから

Updated by mame (Yusuke Endoh) over 12 years ago

遠藤です。

2011年12月4日3:19 KOSAKI Motohiro :

r33886 を ruby_1_9_3 にバックポート希望です。
([ruby-list:48558] のバグ修正)
誰かレビュー頼みます。

一瞬レビューしようと思ったけど、これは無理。あきらめた。
コアなコミッタの降臨を期待します

ぎゃー、r33935 の間違いです。(タイトルの方が正しい)

ところでバックポートチケットに修正内容書かずにレビューワーほいほい
しようとするライフハックはどこ由来なんでしょう。正直僕はあんまり
うれしくないなあ・・・

テスト以外は 1 行パッチなので、ML 番号を書いておけば
十分だと思ってしまいました。

あと正直なところ、バックポートに関しては、されないなら
されないで別にいいや、という気持ちもあります。チケット
作るのも面倒でしょうがなかったのにがんばったという感じ
なので、細かいこと言わないでというか、過剰に期待しない
でほしい。チケット作る気持ちすらなくなっちゃう。

--
Yusuke Endoh

Updated by nagachika (Tomoyuki Chikanaga) over 12 years ago

近永です。

そんなにコアじゃないですがレビューしました。

  • continue するよりは NameError 発生を else 節にしたほうが見通しが良いかと思います。好みの問題のような気もしますけど。
  • また NameError 発生時に、1つ以上の constant の flag を修正済みの場合に rb_clear_cache_by_class() が呼ばれていないことに気がつきました
    というわけで追加の変更を添付します。

あと kosaki さんがおっしゃってたのは、差分の具体的な解説ではなくて(それがあったほうが良いのはもちろんでしょうけど)、「これは○○の件についてのbackport」と一言あると、日常的にチェックしてる人ならだいたいどの修正のことか見当がつくので自分が読めるかどうかすぐ判断ができていいということかなーって思います。この件なら「private constant のアレ」くらいで充分かと。違ってたらごめんなさい。

Updated by nagachika (Tomoyuki Chikanaga) over 12 years ago

すみません、先程わたしが添付したパッチをあてると、なぜか Syck のテストが1つ Failure になってしまいました。後程調べるのでとりあえずこのパッチは差し戻させてください。

Updated by nagachika (Tomoyuki Chikanaga) over 12 years ago

一度 make clean したら発生しなくなりました。
おさわがせしました。

Updated by naruse (Yui NARUSE) over 12 years ago

2011年12月5日12:39 Yusuke Endoh :

ただ、バックポートチケットを書く人に過剰な期待をしないでという気持ち
には変わりがありません。ある程度真面目に書かないといけないとなると、
面倒で後回しにして、そのまま忘れることうけあいです。
でも、いっぱいレビューしてくれてる (し、この先もしてくれそうな)
kosaki さんが、「解読不能なバックポート依頼が来ると、他のレビューも
する気もなくなる」というなら、それはとてもまずいので、どうしても
バックポートしてほしいものだけを真面目にチケット化するようにします。

チケット作るなら簡単に内容を〜というは同意するのですが、
(というかコミットログコピペでいいはずではある)
そもそもバックポートチケットって受益者が作るべきなんじゃないかなぁとは思ったりも。

--
NARUSE, Yui

Updated by kosaki (Motohiro KOSAKI) over 12 years ago

言い訳にしか聞こえませんが私が望んでいるのは「〜の件」と1行書いてほしい
だけで詳細な説明は不要だと思います。どうせみんなcommit log 読んでるんだし。
ChangeLogをカット&ペーストとかでも十分。さすがにリビジョン番号は暗記できない
からつらいなーとかそういう要望でした。

書いた後に気づいたけど、このメール内容がなるせさんのメールとまるまる一緒だ。
すいません、破棄してください。無駄にトラフィック増やしてすいません

Updated by shyouhei (Shyouhei Urabe) over 12 years ago

On 2011年12月05日 14:25, NARUSE, Yui wrote:

2011年12月5日12:39 Yusuke Endoh :

ただ、バックポートチケットを書く人に過剰な期待をしないでという気持ち
には変わりがありません。ある程度真面目に書かないといけないとなると、
面倒で後回しにして、そのまま忘れることうけあいです。
でも、いっぱいレビューしてくれてる (し、この先もしてくれそうな)
kosaki さんが、「解読不能なバックポート依頼が来ると、他のレビューも
する気もなくなる」というなら、それはとてもまずいので、どうしても
バックポートしてほしいものだけを真面目にチケット化するようにします。

チケット作るなら簡単に内容を〜というは同意するのですが、
(というかコミットログコピペでいいはずではある)
そもそもバックポートチケットって受益者が作るべきなんじゃないかなぁとは思ったりも。

受益者というかfirst reporterは当然リリース済みの1.9.3に対してレポート
してくるわけで、それをこちらの都合でtrunkで修正しておいてからもう一回
チケット切り直せと言い出すのはさすがに違うんじゃないですか。

Updated by kosaki (Motohiro KOSAKI) over 12 years ago

チケット作るなら簡単に内容を〜というは同意するのですが、
(というかコミットログコピペでいいはずではある)
そもそもバックポートチケットって受益者が作るべきなんじゃないかなぁとは思ったりも。

受益者というかfirst reporterは当然リリース済みの1.9.3に対してレポート
してくるわけで、それをこちらの都合でtrunkで修正しておいてからもう一回
チケット切り直せと言い出すのはさすがに違うんじゃないですか。

今回の件に限っていうと、ちょっとこのコメントははずしているような。
first reporterがチケットを切っている場合は、そのチケットがそのまま
バックポートチケットに流用されるのでどの話かわからない問題は発生してなくて、
これが起きるのは元チケットがないとき、つまり

1)first reporterがチケットを切らなかった
2)開発者が自分でバグを見つけて、いきなりコミットした

の2択なのです。今回の遠藤さんのケースは(1)ですね。MLで質問が
きて、チケット作成をすっ飛ばしていきなりコミット。

Updated by shyouhei (Shyouhei Urabe) over 12 years ago

On 2011年12月05日 16:17, KOSAKI Motohiro wrote:

今回の件に限っていうと、ちょっとこのコメントははずしているような。

そうかもしれません。ごめんなさい。私がいいたかった事はまた別の機会にします。

Updated by mame (Yusuke Endoh) over 12 years ago

遠藤です。

本題の方、時間があいてしまってすみません。

2011年12月5日10:14 Tomoyuki Chikanaga :

そんなにコアじゃないですがレビューしました。

  • continue するよりは NameError 発生を else 節にしたほうが見通しが良いかと思います。好みの問題のような気もしますけど。
  • また NameError 発生時に、1つ以上の constant の flag を修正済みの場合に rb_clear_cache_by_class() が呼ばれていないことに気がつきました
    というわけで追加の変更を添付します。

今更ですがパッチ見ました。いいと思います。
make check も問題ありませんでした。
コミットをお願いしてもいいでしょうか。

--
Yusuke Endoh

Updated by nagachika (Tomoyuki Chikanaga) over 12 years ago

ありがとうございます。
テストを追加して r33987 でコミットしました。

Updated by kosaki (Motohiro KOSAKI) about 12 years ago

  • Subject changed from backport r33935 to backport r33935, r33987 (fix private constant)

Updated by naruse (Yui NARUSE) about 12 years ago

  • Status changed from Open to Closed

r34538 で backport しました。

Updated by naruse (Yui NARUSE) about 12 years ago

  • Status changed from Closed to Open
  • Assignee deleted (mame (Yusuke Endoh))

r34538 でテストに失敗するようになったので revert しました

Updated by nagachika (Tomoyuki Chikanaga) about 12 years ago

rb_to_id() の引数が trunk で変更されていたので SEGV していました。修正したバックポート用パッチを添付します。

Actions #19

Updated by naruse (Yui NARUSE) about 12 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r34579.
Yusuke, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 33935,33936,33987: [Backport #5702]

* variable.c (set_const_visibility): Module#private_constant has
  changed the visibility of only the first argument.  Now it changes
  all of them.  [ruby-list:48558]

* test/ruby/test_module.rb: add a test for above.

* variable.c (set_const_visibility): print a warning when no argument
  is passwd to Module#private_constant.  [ruby-list:48558]

* vm_method.c (set_method_visibility): ditto for
  Module#private_class_method.

* variable.c (set_const_visibility): clear inine-cache when constant's
  visibility is modified. [ruby-dev:44929]

* test/ruby/test_module.rb (test_private_constants_clear_inlinecache):
  add test for it.
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0