AquesTalkライク記法によるTTS関数の追加とそれに伴うバグの修正 #101
Conversation
There was a problem hiding this comment.
@Yosshi999 さん、 @Oyaki122 さん、 @qwerty2501 さん、もしよければレビューお願いできるととても嬉しいです･･･！
|
size_tの利用、ポインタ渡しから参照渡しへの変更等行ってみました。 |
|
LGTM |
core/src/core.h
Outdated
| * @param output_wav 音声データを出力する先のポインタ。使用が終わったらvoicevox_wav_freeで開放する必要がある | ||
| * @return 結果コード | ||
| */ | ||
| VOICEVOX_CORE_API VoicevoxResultCode voicevox_tts_from_aquestalk_notation(const char *text, int64_t speaker_id, |
There was a problem hiding this comment.
あ、念の為にaquestalkは関数名に含めない方針にしておくとトラブルが無くてよいのかなと感じました！
エンジンのときは kana 引数だったので、真似ると from_kana とか･･･？
| size_t char_size; | ||
| std::string letter = extract_one_character(phrase, base_index, char_size); |
There was a problem hiding this comment.
char_sizeは返り値ですよね、一般的じゃないのかもですがstd::pairやtupleでreturnすれば左辺に書けるので見やすくなるのかもと思いました。
たぶんこう書けるはず
auto [letter, char_size] = extract_one_character(phrase, base_index)There was a problem hiding this comment.
それをやるのであれば、専用のreturn用のstructや構造体を作ったほうがわかりやすいのではないですか？
tupleなどは型情報しか示せるものがないので何番目のfieldが何を示しているかがわかりにくいと思います。
There was a problem hiding this comment.
変数名になるかフィールド名になるかの違いなので、実装されてさえいればまあどっちでもいいのかなと思いました。
There was a problem hiding this comment.
変数名になるかフィールド名になるかの違いなので、まあどっちでもいいのかなと思いました。
There was a problem hiding this comment.
結構違いはあります。
tupleで返して変数で受け取る場合、呼び出し側が適切に意味を理解して明示的に変数名をつけなくてはなりません。
対してstructで返す場合field名をつけるのは関数実装者のためその関数を呼び出す側はfield名を見るだけでそれがどういった意味を持っているか知ることができます。
これは例えば新規で開発に携わる人が何も知らない状態で extract_one_character を呼び出す場合にその戻り値がどういった意味を持っているか理解する手助けにはなります。
もちろんtupleで返す実装の方が extract_one_character を実装する側は楽だとは思いますが
There was a problem hiding this comment.
私見なので私の意見を採用するかはおまかせといった感じです！ 👍
There was a problem hiding this comment.
ちなみにC++の場合参照を渡すほうが適切な場合もあります。戻り値のサイズが大きすぎる場合。
今回はそこまでサイズは大きくないのでその必要はないと思いますが
There was a problem hiding this comment.
ちなみに理由は、 Hoge hoge = hogeFunc() だとコピーが発生するためでしょうか？
初期化の際はstd::moveになるのかなと思っていたのですが、もしかして関数の戻り値は初期化の際もコピー･･･？
There was a problem hiding this comment.
ちょっと説明がややこしいんですが、結論から言うとコピーが発生するためですね。
初期化の際はstd::moveになるのかなと思っていたのですが、もしかして関数の戻り値は初期化の際もコピー･･･？
正確にはstd::moveになるではなくて右辺値になるですね。まあmoveされた結果右辺値になるので概ね解釈としてはあってると思います。これは関数の戻り値も同様です。
上記の例だと例えばHoge classにstringやvectorのフィールドがあったとしてもその各フィールドで確保されてるheap領域についてはmoveされるのでコピーは発生しません。
しかしスタックメモリの実装上関数が終わるとその関数のスタック領域は破棄されてしまうので hogeFunc の戻り値の型が ポインタや参照でもなく Hoge そのものを返すものだった場合どうしても hogeFunc のスタック領域から呼び出し側のスタック領域へのコピーは発生してしまいます。そのため Hoge classのフィールドがint64のものを沢山持っているなどclassのサイズが大きな場合はC++でも出力用の引数を参照で受け取るような実装はありえます。
もちろん hogeFunc の戻り値の型を unique_ptr<Hoge> といった風にheap領域で確保した状態でreturnするという回避方法もあります。(このプロジェクトだとこっちのほうがあってるかも)
There was a problem hiding this comment.
おお、なるほどです。
最近のコンパイラだとスタック領域の移動がなくなるようにコンパイルしてくれたり･･･？
だとしてもコンパイラ依存になっちゃうので、明示的にunique_ptr使ったりしたほうが良さそうに感じます。
勉強になりました。
後から見る方のメモとして、結論としては、実装者に依存してpairで返すのも構造体で返すのもどっちでも良いという感じです！
内容
題の通りです。
元々AquesTalkライク記法を解釈するための関数(
parse_kana)を備えていましたが、それが盛大にバグを含んでいたので、同時に修正です。こちらに動作確認用のテストコードを置いています。
https://gist.github.com/y-chan/91f180f525be2415f7785b545e5011c1