-
Notifications
You must be signed in to change notification settings - Fork 118
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
C APIのセーフティネットのメッセージを改善する #625
C APIのセーフティネットのメッセージを改善する #625
Conversation
あ、 |
なんだこれ... (手元のSphinx v5.3.0はちゃんと動いている) https://github.com/VOICEVOX/voicevox_core/actions/runs/6355234113/job/17263086343?pr=625 追記: #626 で解決 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
これについて補足すると、「重複」が起きる時点でUBであり、それはテストされるべきではないからです。 テスト側から構造体中のフィールドを弄るという手もあるかもしれませんが、何に対するテストかわからなくなるかと思いました。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
めちゃくちゃ素人なコメントを1つしてます。それが問題なければmergeしちゃって頂けると!!
let ptr = s.as_ptr(); | ||
let duplicated = !owned_str_addrs.insert(ptr as usize); | ||
if duplicated { | ||
panic!( | ||
"別の{ptr:p}が管理下にあります。原因としては以前に別の文字列が{ptr:p}として存在\ | ||
しており、それが誤った形で解放されたことが考えられます。このライブラリで生成した\ | ||
オブジェクトの解放は、このライブラリが提供するAPIで行われなくてはなりません", | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
意図的に重複を引き当てるケースを作ることが難しいことに気付きました。
たぶんダメなんだろうなと思いつつのコメントなのですが、同じ変数を2回指定するのはだめですか。
let s: CString = "hoge"; // これで動くかわからないのですが、適当なCStringを定義したいくらいの意図です
whitelist(s);
whitelist(s);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice_owner
の方は厳しいですが、drop_check
の方だと(例示して下さったコードのままだと無理ですが)確かにいけますね!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice_owner
の方も、Allocator
が安定化(stabilized)してくれればそれっぽいアロケータを作ってVec
の第2型引数に入れればいけるんですけどね。今のstable Rustで挿げ替えることができるのはGlobalAlloc
だけなので、例えプロセスを分離した上であってもSliceOwner
の中で使っているBTreeMap
が死んでしまう... (それ以前にtest harness自体とかも死ぬ可能性すらある)
内容
関連 Issue
Resolves #544.
ref #545
その他
と書いていたのですが、意図的に重複を引き当てるケースを作ることが難しいことに気付きました。そのためメッセージの変更のみです。