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

Array::fetch の未定義動作を修正 #990

Merged
merged 4 commits into from Apr 7, 2023
Merged

Array::fetch の未定義動作を修正 #990

merged 4 commits into from Apr 7, 2023

Conversation

tomolatoon
Copy link
Member

Array::fetch の返り値が参照であるため、defaultValueが pvalue への参照である場合にダングリング参照を発生させ、未定義動作を起こしていました。返り値の型を参照から値へと変更し、未定義動作を回避するようにしました。

参照: https://discord.com/channels/443310697397354506/443310697397354508/1092358956443254794

@Raclamusi
Copy link
Member

Raclamusi commented Apr 3, 2023

Array<bool>::fetch()Grid::fetch() についても同様の問題があるので、同時に変更すべきかと思います。

@Reputeless
Copy link
Member

Reputeless commented Apr 4, 2023

ありがとうございます! コミットいただいた内容で Siv3D の問題を解決できます。
ついでですが、戻り値が value_type になったので、std::optional<T>::value_or() のように引数を (U && defaultValue) とする ことで、さらに実行時性能の改善ができ一石二鳥の pull request になります。

Array<String> vs = { U"aaa", U"bbb" };
String s = vs.fetch(0, U"hoge"); // String(U"hoge") を構築しないようにできる

合わせてコミットいただけるとありがたいです。

参考実装
https://github.com/microsoft/STL/blob/main/stl/inc/optional#L423-L436

@Reputeless
Copy link
Member

ありがとうございます。

Array<Color> v;
v.fetch(0, U"#FFFFFF");

のコンパイルを通すために、いずれも return static_cast<value_type>(std::forward<U>(defaultValue)) とするのが良さそうです。この変更が反映され次第マージします。

@Raclamusi
Copy link
Member

そうすると

Array<Array<int32>> v;
v.fetch(0, 42);

のようなコードが有効になりますが、これは非直感的ではないですか?

@Reputeless
Copy link
Member

ご指摘ありがとうございます。
確かに標準の std::optional::value_or() を見ると is_convertible_v<U, T> == true が要件になっています。
標準に合わせて、ここは変更しないほうが良いです。

@Reputeless Reputeless merged commit 4a1ea87 into Siv3D:v6_develop Apr 7, 2023
1 check passed
v0.6 Roadmap automation moved this from ToDo to Done Apr 7, 2023
@Reputeless
Copy link
Member

Merged. Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants