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.

Sign up for GitHub

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

Jump to bottom

use safer text2mecab #119

Closed
wants to merge 1 commit into from
Closed

use safer text2mecab #119

wants to merge 1 commit into from

Conversation

Yosshi999
Copy link
Contributor

内容

VOICEVOX/open_jtalk#4 に追従

text2mecabが失敗した時は例外を投げる

関連 Issue

#112 (comment)

現在のcore.dll実装には voicevox_tts に長すぎる文字列を与えるとbuffer overflowでクラッシュする

これを解決

その他

@Yosshi999 Yosshi999 mentioned this pull request Apr 21, 2022
Merged
y-chan
y-chan approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member

@y-chan y-chan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!
どうやら一部のテストが落ちてるみたいです...?
原因がいまいちわかりませんが、直すべきなのはopenjtalk側...?

@Hiroshiba
Copy link
Member

あ、ほんとですね！！
EINVALという定数はwindows限定なのかな。

@Yosshi999
Copy link
Contributor Author

Yosshi999 commented Apr 22, 2022 •
edited
Loading

うーん？印象的にはlinuxのものだと思ってたのでwinしか通らないのは謎ですね
includeすべきファイルが違うとか？

なんかOS依存っぽいのでerrno_t使うのやめます

@Yosshi999 Yosshi999 mentioned this pull request Apr 29, 2022
Merged
@Yosshi999
Copy link
Contributor Author

VOICEVOX/open_jtalk#5

これが入ったらこっちに反映します

@PickledChair
Copy link
Member

PickledChair commented May 2, 2022 •
edited
Loading

VOICEVOX/open_jtalk#5

これが入ったらこっちに反映します

こちらの PR をレビューしましたが、書き込み権限が設定されていないようでマージできませんでした。 @Hiroshiba さんに解決をお願いしたいと思います！ マージしました！

EINVALという定数はwindows限定なのかな。

後学のために自分でも少し実験してみましたが、Windows では string.h を include するだけで EINVAL が使えるようになりました。他の OS では errno.h を include しなければなりません。これが原因で Windows だけビルドが通ったようです。それはそれとして、POSIX 規格とは関係なく自前でエラー番号を定義する方法（ VOICEVOX/open_jtalk#5 のアプローチ）の方が問題が少ないと自分も感じました。

@Hiroshiba Hiroshiba mentioned this pull request Jun 28, 2022
Closed
43 tasks
@qwerty2501
Copy link
Contributor

このPRについてはC++時代のものであり、もう取り込めない変更になってるのでCloseします。
もんだいがあればreopenしてください

@qwerty2501 qwerty2501 closed this Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-chan y-chan y-chan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Yosshi999 @Hiroshiba @PickledChair @qwerty2501 @y-chan