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

OpenJtalkSynthesizer<OpenJtalk> | Synthesizer<()>として持つ #694

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Nov 26, 2023

内容

struct OpenJtalkSynthesizer<OpenJtalk> | Synthesizer<()>として持つようにします。
(Rust APIはパブリックではないので暫定)

これにより、OpenJtalkは必ずシステム辞書を読み込んでいる状態になります。

変更しているpublic APIはRust APIのみです。C(compatible_engineを除く)もPythonもJavaも現状Open JTalkが必須となっているのですが、もし必須ではなくするとしたら、設計については議論の余地があると思っています。

考慮すべきはこんな感じだと思っていて、

  • アドホック多相(Synthesizer<()>からはメソッドが生えなくて、Synthesizer<OpenJtalk>からは生えてくる)があると仮定しない
    • Rust APIはまた組み直す
  • classの継承があると仮定しない(Rust以外にもGoとかがある)
  • 関数の引数や戻り値に動的ディスパッチされるinterfaceが使えると仮定しない
  • interfaceとかabstract classに該当するものは、現代的な言語には流石にあるはず
    • classの継承があるなら、親側をabstractということにすればいけるはず。例えばOCamlやSML#でも多相バリアントで部分型付けが表現できるのでどうにかなるはず(参考
  • 言語間で多少形が違うくらいは許容してもよいはず。例えばCは、void*で扱って実行時チェックとするしかないはず

これをもとにしても、私が今思い付くだけでこれくらいの選択肢はあると思います。

class Synthesizer (OpenJtalkはオプショナルなオブジェクトとして持ち、null(相当)であるときにメソッドを呼ぶと実行時エラー)
class SynthesizerWithoutOpenjtalk (互いに親子関係には無い)
class Synthesizer                 (〃)
class Synthesizer
└── class SynthesisEngine (`Synthesizer::engine`として所有)
interface SynthesisEngine (存在型か、動的ディスパッチでコンストラクトする)
└── class Synthesizer
interface SynthesisEngine            (引数のみに登場し、返り値としては登場しない)
├── class SynthesizerWithoutOpenjtalk
└── class Synthesizer

関連 Issue

#545

その他

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!!!

綺麗な実装だと思いました!!

これをもとにしても、私が今思い付くだけでこれくらいの選択肢はあると思います。

色々選べる言語の場合どれがいいか結構迷いますね。
うーん。片方が片方を参照する形か、nullとして持っておいて実行時エラーか、ですかねぇ。。
(言語ごとにOpenJtalk有無で2クラス実装するのは大変そう)

@Hiroshiba Hiroshiba merged commit 67640c1 into VOICEVOX:main Nov 27, 2023
31 checks passed
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.

2 participants