Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl Iteratorimpl IntoIterator #11

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Apr 3, 2023

タイミングが今で申し訳無いのですが、 #9 で入ったコードに対する提案です。

こちらの方がidiomaticかと思います。

@PickledChair
Copy link
Member

なるほどです! 言われてみれば、なんとなくこちらの型指定の方が多いかもですね。

後学のためにお聞きしたいのですが、この型指定にすることで得られるメリットが明らかにある場合教えてくださると嬉しいです。また、このような「こう書いた方がより良い」という指針はどこから得られましたか? なんとなく、確かに標準ライブラリはこんな感じの引数にしてあるなと思い当たったのですが……。

@qryxip
Copy link
Member Author

qryxip commented Apr 3, 2023

この型指定にすることで得られるメリットが明らかにある場合

利点としてはVec<T>&'a [T]のような型の値を直接引数に入れられることでしょうか? ただimpl Iteratorが良いというよりもimpl Iteratorだと中途半端ではないかと思ったのがPRの動機です。私が1から書いたら&[impl AsRef<Path>]とか&[PathBuf]とかにしていたかもしれません。

#12 もですが、 #9 のレビュー中だったらsuggestion投げていたかなというくらいの温度感でいます。

また、このような「こう書いた方がより良い」という指針はどこから得られましたか?

あえて出典を挙げるならRust API GuidelinesのC-GENERICでしょうか? まあこれはpublicなAPIについての話ですが。

@PickledChair
Copy link
Member

なるほどです、ありがとうございます。

私が1から書いたら&[impl AsRef<Path>]とか&[PathBuf]とかにしていたかもしれません。

これは確かに私もレビューしているときに同様のことを感じました。しかし「変えなければいけない」というほどの理由が自分の中にはなかったのでそのままにした感じです。役割を十分果たしていればそれで問題ない、と判断しました。この関数は他の場所で再利用される場面はほぼないと考えて良さそうだったのと、仮にあったとしてもそのときに適切な形に変えれば良い、という判断もありました。

(正直にいうと「コミュニケーションの数を最適化したい」という気持ちもありました。個人的に最近忙しくなってきているので、時間的・体力的なリソースを潤沢に振り向けられなくなってきています。ここはちょっと申し訳なさがあります。明らかに重大な議題であったならそれでも十分議論を尽くすつもりでいますが……)

利点としてはVec<T>&'a [T]のような型の値を直接引数に入れられること

という意見は確かにそうだな、と感じ勉強になりました。参考リンクもありがとうございます。ただ上に書いた観点からは、個人的には「変更前でも変更後でもどちらでも良い」という印象でした。

(誤解を避けるため念のためお伝えしますが、今回のように「この方がより良いかも・より自然かも」という提案や視点をいただけること自体は非常にありがたいと思いました。)

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的には「変更前でも変更後でもどちらでも良い」という印象でした。

という意見なので、逆にいうと問題は全然ないと感じたので approve にします

@qryxip
Copy link
Member Author

qryxip commented Apr 3, 2023

これは確かに私もレビューしているときに同様のことを感じました。しかし「変えなければいけない」というほどの理由が自分の中にはなかったのでそのままにした感じです。役割を十分果たしていればそれで問題ない、と判断しました。この関数は他の場所で再利用される場面はほぼないと考えて良さそうだったのと、仮にあったとしてもそのときに適切な形に変えれば良い、という判断もありました。

なるほど。レビュー判断としてそのままにしたんですね。

(正直にいうと「コミュニケーションの数を最適化したい」という気持ちもありました。個人的に最近忙しくなってきているので、時間的・体力的なリソースを潤沢に振り向けられなくなってきています。ここはちょっと申し訳なさがあります。明らかに重大な議題であったならそれでも十分議論を尽くすつもりでいますが……)

そこはお時間取ってしまって申し分けなかったなーと思ってます。

実はこのPRは #12 と合体して"Fix up #9"のようなタイトルで出そうと思ってたのですが、qwertyさんが書いていた部分で一箇所だけ.display()が使われている所があったためPRを分離したんですよね。

変えるべき理由は特に無い...けど変えない理由も特に無いし、レビュー中ならsuggestion出してた。 #9 マージ直後の今ならまだセーフなのでは?という思考でこのPRを出しました。こう、なんというか3秒ルール的な感じで (本物の3秒ルールは衛生的に駄目ですが)。

@PickledChair
Copy link
Member

あー、なるほどです。本来は先の PR と同時に改善しようとしていた、という事情がわかりました。なんにせよ、提案自体はなるほどなと思ったのでありがとうございます!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちゃんとわかってないけどLGTM!!

@Hiroshiba Hiroshiba merged commit 4a81257 into VOICEVOX:main Apr 3, 2023
@qryxip qryxip deleted the impl-iterator-to-impl-intoiterator branch April 3, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants