[project-vvm-async-api] いくつかのC関数を定数にする #503

qryxip · 2023-05-27T03:57:49Z

内容

C APIのいくつかの関数を定数にします。

voicevox_get_version() → voicevox_version
voicevox_make_default_…_options() → voicevox_default_…_options (新クラス設計API #370 (comment))
~~voicevox_get_supported_devices_json() → voicevox_supported_devices_json? ([project-vvm-async-api] get_supported_devices_jsonをfallibleに #502 (comment))~~

その他

qryxip · 2023-05-27T03:59:03Z

crates/voicevox_core_c_api/src/lib.rs

-
-/// voicevoxのバージョンを取得する
-/// @return SemVerでフォーマットされたバージョン
+/// voicevoxのバージョン


なんかcbindgenがこういう定数へのdocstringを拾ってくれない...

悲しいですね･･･
そのオプションを使う関数の方にメモを書くことになるのかなと思いました。。 😇
（ユーザー的にはそちらの方が便利かも）

ちょっと面倒かもしれないんですけれどもお願いできますか 🙇‍♂️

cbindgenのドキュメントを探しても何故こうなっているかの理由は見つけられませんし、issueもそれっぽいものは無さそうです。#defineによる定数だとちゃんとrustdocを引き継ぐのも変ですし、もう少し調べてみます。

/// ああああああああああ pub const N: i32 = 42; /// ああああああああああ #[no_mangle] pub static M: i32 = 42;

↓

/** * ああああああああああ */ #define N 42 #ifdef __cplusplus extern "C" { #endif // __cplusplus extern const int32_t M; #ifdef __cplusplus } // extern "C" #endif // __cplusplus

不思議ですね･･･。
よくよく考えたらstaticにしておく理由は特にない？ので、define(const)でもいいかもとかちょっと思いました。
全部大文字にしないと不自然かもで、可読性は落ちるかもですが 😇

#defineにしたらヘッダー(つまりAPIの「形式」)にだけ"0.15.0"みたいな情報が埋まって、DLL本体から情報が抜けるのではと思います。

意図が読めてないのですが、抜けるから不便or良いどちらでしょうか？

不便の方ですね。

そもそも他言語バインディングを作るときなんかは必ずしもヘッダーを直接読んだりしないでしょうし、読むとしてもOptions系をちゃんと解釈できるか怪しいと思います。あとバージョン不明なvoicevox_core.dllのAPIを叩いてバージョンを調べる、といったことも不可能になると思います。
(追記) compatible_engineとかdeprecatedな方を叩けばいけるかもしれませんが...

なるほど！
たしかにバインディング作るときはたしかにdefineを自分で書かないといけなくなりますね･･･。

諦めるか、関数に戻すか、半分あきらめつつissue建てるかでしょうか。
個人的には半分諦めてissue建てるのが落とし所かなと思いました。

なんか"IR"としてはちゃんとdocstring部分を拾っているようです。

https://github.com/mozilla/cbindgen/blob/3680e4d539e33977d613f161530c91a284a82428/src/bindgen/ir/global.rs#L25

そしてこれを消してもコンパイルは通る!!!

crates/voicevox_core/build.rs

crates/voicevox_core_c_api/include/voicevox_core.h

qryxip · 2023-05-28T08:29:21Z

crates/voicevox_core_c_api/include/voicevox_core.h

型パートでの型の順番が何故か入れ替わっている...

crates/voicevox_core/src/result_code.rs

crates/voicevox_core/src/version.rs

qryxip · 2023-05-28T08:35:41Z

crates/voicevox_core_c_api/src/lib.rs

アイテムの順番が.hでのものと異なっている(cbindgenは型→定数→関数の順で並べる)ので、.h側に合わせる形で並び換えるのがいいかもしれない。

関数とかの属性よりも扱っている概念でまとまっていたほうがわかりやすいだろうなーとは思いつつも、ヘッダー側と並び順が違うのはあとで見づらい気もしますねぇ。
まあヘッダーと並び順が違うことで困る人は珍しいし、個人的にはこのままのが良いのかなとか思いました。

並び順自体が違うこと自体はいいとして、ヘッダー側の並び順を制御するのに今後困ることがありそうな予感がしています。今回こういう↓よくわからないことが発生していますし。
#503 (comment)
どっちにしろmainとの差分は凄いことになるので、もうdiffが60%を超えることを前提にがっつり入れ替えてもいいんじゃないかと思いました。

もう一つの方針として、(そもそもできるかどうかわかりませんが)ヘッダー側を「扱っている概念でまとまってい」る並び順にするというのが考えられますが、最終的にはこういう形のドキュメントを生成してユーザーに提供することになるので、無理が出てくると思います。

基本的にC側に合わせるのは得策ではないと思っています。
ユーザーは困らない、API追加のたびにヘッダー生成して順序合わせる作業が発生して大変、コードが読みづらくなるためです。
順序違いは諦めるか、C側をRust側に合わせるのを目指すのはどうでしょう。

論点がいくつかあっていろいろ混じってるように思うので考えを整理して書いてみました。

Rust側とC側でドキュメントの順序が異なるのをどうすべきか

合わせないで良いと思う

困る人がいないため

ヘッダーの順序は生成されるまで不明で新規コミットが大変なため

どちらかというとC側が古い

関数等の型で並べるよりエンジンAPIみたくドメインで並べたほうがいい

合わせられるなら合わせたい

整合性取れてるか判断するためにコミッターとレビュワーがちょっとなため

その2つだと諦める方向ですかね...
C側の順序をなんとかしようとするとcbindgenとDoxygenの両方をどうにかしないといけないので、大変な気がします。

実装の難度やユーザーの需要はともかく、メンテコストの認識には微妙に齟齬がありそうな気がします。

ヘッダーに関しては #493 でこのリポジトリに追加されているため、手元で

❯ cargo xtask update-c-header

ってやれば更新できるのでそれをgit diffで見ればいいと思います(readmeに一応書いてありますし、CIも回っています)。私は今実際にそうやっています。

コードの可読性については、そもそもlib.rsはAPI視点のシグネチャとドキュメントを中心にした方がいいのではないでしょうか。今あるhelpers.rsとc_impls.rsの2つは実装を分離させるためにあるはずです。

❯ tree crates/voicevox_core_c_api/src/ crates/voicevox_core_c_api/src/ ├── c_impls.rs ├── compatible_engine.rs ├── helpers.rs └── lib.rs 1 directory, 4 files

実装→ヘッダー作成→順序変える、ですよね。たしかにそこまで手間でも無いかもですが、意味が薄い作業（困る人がいないため）を手間かけてやらないといけないのはデメリットではあると思います。

コードの可読性については、そもそもlib.rsはAPI視点のシグネチャとドキュメントを中心にした方がいいのではないでしょうか

ドキュメントは使う側のために作っているもので実装側はあまり見ないので考えなくて良いはずです。
実際エンジン側ももう１年ほどずっとドキュメント順序と実装順序が別ですが、問題だと感じたことがなかったです。

実装順序とドキュメントの順序が違うの、全然普通な気がしてきました。
例えばPythonのpathlibを見た感じ、実装とドキュメントで全然違いそうです。

実装とドキュメントが違うのは一般的だとは思います。Rustdocもソースコード上の順番に関わらずアルファベット順になりますし。私が問題だと思っているのはCでは上から下に定義を書かなくてはいけないために、このPRのようなことをするとcbindgenが荒ぶることです。

いっそのことCヘッダー側をアルファベット順でソートするのはどうでしょうか? cbindgenにはそういうオプションはあります。ドメインの区分けがわからなくなりそうですが、ドキュメントをコード例と共に手厚く書いてquickstart的なものも用意すれば大丈夫なはずだと思います。

なるほどです。今回のように型が変わって順番が変わるのは破壊的変更があるときくらいなので、そもそも破壊的変更が大きいから問題とまではいかない気がしました。

名前順でも別に良いと思います。
ユーザーにとって一番困るのは運営が明確な意図なくポンポンいろいろ変えてくることだと思うので、しっかり意図を考えておきたいかもです。
「今後ドキュメントの見た目が大きく変更されなそうだから」とかですかね。

crates/voicevox_core/src/version.rs

crates/voicevox_core/src/voice_synthesizer.rs

Hiroshiba

いくつかコメントしていますが、ほぼLGTMです！！

…some-functions-constants

qryxip · 2023-06-03T09:04:32Z

#507 のコンフリクト解消はしたので、あとはversionのあれこれの取りやめですね
(コンフリクト解消はiPadのブラウザでやりました)

qryxip · 2023-06-03T09:06:13Z

あーduplicateクレートのやつもありましたね。draftにしときます

crates/voicevox_core/src/version.rs

Hiroshiba

あとはstaticのドキュメント書かれないのどうするかと、ヘッダーの並び順変わるのどうするかが議論ポイントって感じでしょうか。
いったんプルリクのタイトルのことは達成できてますし、とりあえずマージしちゃってタスクに積んでおくのも別にいいかな〜と思います。
@qryxip さんのやりやすい方で･･･！！！

qryxip · 2023-06-06T13:53:29Z

そうですね。タスクリストに入れておきました。cbindgenは腰を入れて調べないといけない気がします...

…some-functions-constants

Hiroshiba

LGTM！！！

Rustの勉強になりました、ありがとうございました！！

Hiroshiba · 2023-06-07T01:07:08Z

@PickledChair Rust周りのこととかコメント頂けると心強いのですが、お時間どうでしょう･･･？ 👀

qryxip · 2023-06-07T11:57:27Z

このPRを実装するにあたり、soundnessについて私が考えていたことです。
まあこの辺は書かんでもいいでしょと思っていたのですが、SuitCaseさんがレビューしてくださるなら取り急ぎ書いておきます:

no_mangleな…Optionsの定数を宣言してよいのか?
- よい。プリミティブ型でなければならないといったルールは無いはずで、"FFI-safe"であるならよいはず
no_mangleな&strの定数を宣言してよいのか?
- よい。&Tは*const Tとして扱える
  - もっと言えばOption<&T>とかも*const Tとして扱ってよい
  - alignedかつShared XOR Mutableルールから逸脱していなければ*const Tは&Tとしてよい

不明な点や他の疑問があったら遠慮なく聞いて下さい。

PickledChair · 2023-06-07T13:39:47Z

@Hiroshiba

@PickledChair Rust周りのこととかコメント頂けると心強いのですが、お時間どうでしょう･･･？ 👀

ちょっと思ったより最近忙しいので時間が取れてなくてすみません。ざっとみた感じでは問題なさそうでした……！

@qryxip

この辺は書かんでもいいでしょと思っていたのですが、SuitCaseさんがレビューしてくださるなら取り急ぎ書いておきます:

情報ありがとうございます！　私も多分問題ないと思います。

PickledChair · 2023-06-07T13:44:32Z

問題ないと思うのでマージします。cbindgen の問題について #503 (comment) の方もありがとうございます

This reverts commit f4b502a. # Conflicts: # Cargo.lock # Cargo.toml # crates/voicevox_core/Cargo.toml # crates/voicevox_core_c_api/Cargo.toml # crates/voicevox_core_c_api/include/voicevox_core.h # crates/voicevox_core_c_api/src/lib.rs

Co-authored-by: Ryo Yamashita <[email protected]> Co-authored-by: kasamatsu <[email protected]> Co-authored-by: shigobu <[email protected]>

voicevox_get_version() → voicevox_version

2a26760

qryxip changed the title ~~いくつかのC関数を定数にする~~ [project-vvm-async-api] いくつかのC関数を定数にする May 27, 2023

qryxip commented May 27, 2023

View reviewed changes

crates/voicevox_core/build.rs Outdated Show resolved Hide resolved

crates/voicevox_core/build.rs Outdated Show resolved Hide resolved

crates/voicevox_core_c_api/include/voicevox_core.h Show resolved Hide resolved

qryxip added 4 commits May 27, 2023 22:17

indocを外す

dc0831a

コメントを更新

c0ece34

テストglobal_infoを追加

0918b58

make_default_系を定数に

78c75b7

qryxip commented May 28, 2023

View reviewed changes

Minor refactor

6927143

qryxip marked this pull request as ready for review May 28, 2023 08:38

qryxip mentioned this pull request May 28, 2023

[project-vvm-async-api] get_supported_devices_jsonをfallibleに #502

Merged

qryxip commented May 28, 2023

View reviewed changes

crates/voicevox_core/src/version.rs Outdated Show resolved Hide resolved

qryxip added 5 commits May 29, 2023 00:51

diffを減らすためにquoteをダウングレード

9baebfb

Minor refactor

a3090c0

Minor refactor

f06c46b

SupportedDevicesの中身も確認する

464adb5

Minor refactor

706fb3d

Hiroshiba reviewed Jun 1, 2023

View reviewed changes

crates/voicevox_core/src/version.rs Outdated Show resolved Hide resolved

qryxip mentioned this pull request Jun 2, 2023

[project-vvm-async-api] C/Python APIクレート側のバージョンを出す #507

Merged

Hiroshiba reviewed Jun 2, 2023

View reviewed changes

crates/voicevox_core/src/voice_synthesizer.rs Outdated Show resolved Hide resolved

Hiroshiba reviewed Jun 2, 2023

View reviewed changes

Merge branch 'project-vvm-async-api' into project-vvm-async-api-make-…

035c9ca

…some-functions-constants

qryxip marked this pull request as draft June 3, 2023 09:07

version!() → VERSION

e0fa587

qryxip commented Jun 4, 2023

View reviewed changes

crates/voicevox_core/src/version.rs Show resolved Hide resolved

Defaultのimplはマクロ側に寄せる

77973d2

qryxip marked this pull request as ready for review June 4, 2023 13:23

Hiroshiba reviewed Jun 6, 2023

View reviewed changes

qryxip mentioned this pull request Jun 6, 2023

project-vvm-async-api #497

Merged

67 tasks

qryxip added 2 commits June 6, 2023 22:59

Merge branch 'project-vvm-async-api' into project-vvm-async-api-make-…

51eacd3

…some-functions-constants

voicevox_create_supported_devices_jsonのシグネチャを直す

721d62e

Hiroshiba approved these changes Jun 7, 2023

View reviewed changes

PickledChair approved these changes Jun 7, 2023

View reviewed changes

PickledChair merged commit f4b502a into VOICEVOX:project-vvm-async-api Jun 7, 2023

qryxip mentioned this pull request Jun 10, 2023

[project-vvm-async-api] mainをマージする #516

Merged

qryxip mentioned this pull request Jul 22, 2023

[2月末リリース予定!] v0.16 #545

Open

69 tasks

shigobu mentioned this pull request Jul 26, 2023

default option系定数をC/C++以外(主にC#)から取得する場合に煩雑になる #549

Closed

3 tasks

shigobu mentioned this pull request Jul 31, 2023

Revert #503 定数化されたものを関数へ戻します #557

Merged

qryxip added a commit that referenced this pull request Aug 13, 2023

Revert #503 定数化されたものを関数へ戻します (#557)

263beb4

Co-authored-by: Ryo Yamashita <[email protected]> Co-authored-by: kasamatsu <[email protected]> Co-authored-by: shigobu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[project-vvm-async-api] いくつかのC関数を定数にする #503

[project-vvm-async-api] いくつかのC関数を定数にする #503

qryxip commented May 27, 2023 •

edited

Loading

qryxip May 27, 2023 •

edited

Loading

Hiroshiba Jun 2, 2023

qryxip Jun 4, 2023

Hiroshiba Jun 4, 2023 •

edited

Loading

qryxip Jun 4, 2023

Hiroshiba Jun 4, 2023

qryxip Jun 4, 2023 •

edited

Loading

Hiroshiba Jun 4, 2023

qryxip Jun 11, 2023

qryxip May 28, 2023

qryxip May 28, 2023

Hiroshiba Jun 3, 2023

qryxip Jun 4, 2023

Hiroshiba Jun 4, 2023 •

edited

Loading

qryxip Jun 4, 2023

qryxip Jun 4, 2023

Hiroshiba Jun 4, 2023

qryxip Jun 4, 2023

Hiroshiba Jun 4, 2023 •

edited

Loading

Hiroshiba left a comment

qryxip commented Jun 3, 2023

qryxip commented Jun 3, 2023

Hiroshiba left a comment

qryxip commented Jun 6, 2023 •

edited

Loading

Hiroshiba left a comment

Hiroshiba commented Jun 7, 2023

qryxip commented Jun 7, 2023 •

edited

Loading

PickledChair commented Jun 7, 2023

PickledChair commented Jun 7, 2023

[project-vvm-async-api] いくつかのC関数を定数にする #503

[project-vvm-async-api] いくつかのC関数を定数にする #503

Conversation

qryxip commented May 27, 2023 • edited Loading

内容

関連 Issue

その他

qryxip May 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qryxip Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hiroshiba Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Hiroshiba left a comment

Choose a reason for hiding this comment

qryxip commented Jun 3, 2023

qryxip commented Jun 3, 2023

Hiroshiba left a comment

Choose a reason for hiding this comment

qryxip commented Jun 6, 2023 • edited Loading

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba commented Jun 7, 2023

qryxip commented Jun 7, 2023 • edited Loading

PickledChair commented Jun 7, 2023

PickledChair commented Jun 7, 2023

qryxip commented May 27, 2023 •

edited

Loading

qryxip May 27, 2023 •

edited

Loading

Hiroshiba Jun 4, 2023 •

edited

Loading

qryxip Jun 4, 2023 •

edited

Loading

Hiroshiba Jun 4, 2023 •

edited

Loading

Hiroshiba Jun 4, 2023 •

edited

Loading

qryxip commented Jun 6, 2023 •

edited

Loading

qryxip commented Jun 7, 2023 •

edited

Loading