CNativeW::SetString に NULL を指定した場合に wcslen に NULL を渡して落ちてしまう不具合を修正 #1087
Conversation
ユニットテストに関しても通るように変更
|
✅ Build sakura 1.0.2371 completed (commit 19296a07d6 by @beru) |
sakura_core/mem/CNativeW.cpp
Outdated
| if (pszData) | ||
| CNative::SetRawData(pszData,wcslen(pszData) * sizeof(wchar_t)); | ||
| else | ||
| Clear(); |
There was a problem hiding this comment.
NullとEmpty(空文字列)を区別して扱えるようにしたいです。
ここの Clear() は型修飾を付けて CMemory::Clear() とすべきだと思います。
CNativeWから修飾なしで Clear() を呼び出すと CNative::Clear() が使われます。
CMemory と CNative では Clear() の意味が違っていて、CNative::Clear() だとメモリを開放しないのでNULLを代入できません。
- 派生関係にある2つのクラスで、同名関数の挙動が違う、というのは混乱のもとです。そういう混乱を防ぐための指針として リスコフの置換原則 というものがあります。
CNative::Clear()の挙動を変更したのは CNative::Clear の実装を改善 #777 で、比較的最近の話です。たしか当時、std::basic_string::clear()の挙動に習って「メモリを開放しない」という拡張を行った気がします。この変更自体を「間違い」とは思っていません。CMemoryのメモリ解放関数の名前が_Empty()なのも混乱の元かもです。CMemoryは「空っぽにできる対象」を2つ持っています。1つ目が「確保したメモリ」で2つ目が「メモリの中身」です。現在のCMemory::Clear()の実装は「確保したメモリ」を「空っぽにする」です。派生クラス側で「メモリの中身」を空っぽにする関数にClear()と名付けているのでややこしくなってる感じです。
There was a problem hiding this comment.
お、そういう違いがあるんですね。認識してませんでした。ややこしいなぁ。
しかし文字列型クラスである CNativeW って今は CNative を継承する作りになっていて、CNative は CMemory を継承していますが、機能を実現するにあたって継承構造にする必要性って無いような気がするんですよね。
ところで、領域確保していない NULL と、領域確保はしているけど文字列の長さが 0 の空文字状態の区別は、外部から区別するとしたら capacity メソッドを使う事ぐらいでしょうかね。
void CNativeW::SetString( const wchar_t* pszData ) に NULL が渡されたときに領域確保していない状態にする事で何かの旨味があるのかはよくわかりません。文字列型として考えた時に使用する外部が使い分けを意識するのは何か面倒くさそう…。
There was a problem hiding this comment.
#1086 の説明を読むと、NULL 代入後に NULL と比較した際に true が返るようにしたい、というのがモチベなんでしょうか？
メモリ領域を確保していない NULL 状態と 領域確保してて文字列の長さが 0 の場合のインスタンスの比較結果は false になるという事ですよね。うーん、文字列型としてその挙動が自然なのかなぁ。抽象的な文字列というより具象的な文字列メモリとして扱う型になりそう。
There was a problem hiding this comment.
#1086 の説明を読むと、NULL 代入後に NULL と比較した際に true が返るようにしたい、というのがモチベなんでしょうか？
う～む。それがモチベか？と問われると微妙です。
-
代入できるってことは、代入後に指定した値と等価になることなんじゃないかと思います。
-
NULLが代入できるってことは、代入後にNULLと等価になるんだと思います。
-
現在はNULLと比較できないので微妙です。
- NULLと比較して等価になるCNativeWはメモリ未確保状態だと思います。
- メモリ確保状態からメモリ未確保となるためにはメモリを開放する必要があります。
-
std::wstringはNULLという値を保持できません。
-
CNativeWはNULLという値を保持できます。
考えようによっては、NULLを保持できることがCNativeWを残して使っていくメリットの1つだと思います。
全部「std::wstringに習って」で進めていくんだとしたら「なんでCNativeWを残してくの？」という疑問がつきまとう気がします。CNativeWを使うメリット、というのはあると思っているので、今後も改善しつつ残していくつもりではいますが。
メモリ領域を確保していない NULL 状態と 領域確保してて文字列の長さが 0 の場合のインスタンスの比較結果は false になるという事ですよね。うーん、文字列型としてその挙動が自然なのかなぁ。
まだ比較演算子は実装できてないんですが、そういうことになります。
NULLとL""は区別したいっす。あとで string.IsNullOrEmpty() を作る羽目になるとしても、です。
抽象的な文字列というより具象的な文字列メモリとして扱う型になりそう。
言葉の意味がわからんかったとです。
抽象的な文字列ってのは、たとえば std::wstring ですかね？
超ざっくりですが、CNativeWとstd::wstringはベツモノだと思っています。
There was a problem hiding this comment.
色々と説明ありがとうございます。NULL と L"" を区別したいという意思は分かりました。それに対して肯定/反対する明確な理由は自分は今のところ持ち合わせていないです。
抽象的な文字列ってのは、たとえば std::wstring ですかね？
超ざっくりですが、CNativeWとstd::wstringはベツモノだと思っています。
C++ の文字列型として有名なのは、std::basic_string や Microsoft の MFC/ATL/WTL 等で使われてる CString でしょうか？ QTを使ってる人はQString なんかも使っていそう。まぁなんとなく文字列の扱いを快適に行える型とイメージしていました、C++に限らず。
CNativeW の内外のサポートが充実すれば快適度が増していくと思うので、そうなれば他の文字列型と気になる違いがあったとしても、それはそれ、これはこれ、と片付けてしまって良いと思います。
まぁ単に文字列型の使用状況を単純化したいなら標準の std::wstring を出来るだけ使うようにすれば良いのかもしれないですけど、そのように変更する流れになっていないという事はやっぱ色々メリットデメリットがあるのかなぁ。。C言語の標準関数使って書かれている部分もたくさんあるし、小回りが利く、とか、過去の経緯で、とか色々な理由で混在してるんでしょうか…。C++の標準ライブラリの文字列への配慮が薄いのは作者の頭髪と関連しているのかもしれないですね。
tests/unittests/test-cnative.cpp
Outdated
|
|
||
| CNativeW value2(nullptr); | ||
| EXPECT_EQ(value2.GetStringLength(), 0); | ||
| EXPECT_EQ(value2.GetStringPtr(), nullptr); |
There was a problem hiding this comment.
expected, actualの書き順が逆っす 😄
というアフォな突っ込みを過去何度か入れた気がしますが、こういう指摘はしないほうがいいんでしょうかね。
There was a problem hiding this comment.
http://opencv.jp/googletestdocs/primer.html#primer-binary-comparison
第1引数が期待値で、第2引数が実際の値にした方がいいんでしょうか？EXPECT_EQ マクロのコメント内の Examples ではそういう記載になっていませんけれど。
There was a problem hiding this comment.
expected, actualの書き順が逆っす 😄
というアフォな突っ込みを過去何度か入れた気がしますが、こういう指摘はしないほうがいいんでしょうかね。
社会の窓が開いてたりシャツの表裏や前後ろを反対に着ている人にする指摘と同じような事かもです。
どういうリアクションになるかは指摘された人の性格によるかもしれません。
実用上問題無いといって指摘をはねのける人がいるかもしれません。
パンツを頭に被っていたり全裸で歩いている人を公道で見かけた場合には…おそらく指摘しないで急いで立ち去った方が良いと思います。警察に通報するとか。
|
✅ Build sakura 1.0.2372 completed (commit b93dc34591 by @beru) |
| * @brief コンストラクタ(nullptr指定)の仕様 | ||
| * @remark 構築できない(実行時エラーになる) | ||
| * @note バグですね(^^; | ||
| * @brief コンストラクタ(NULL指定)の仕様 | ||
| */ |
There was a problem hiding this comment.
このクラスに関して、テストを整備する目的は挙動を仕様化することなので、想定結果をコメントで入れたいっす。
まあ、後で自分でPR出しますんで対応不要ですが。
|
|
||
| CNativeW value2(nullptr); | ||
| EXPECT_EQ(0, value2.GetStringLength()); | ||
| EXPECT_EQ(nullptr, value2.GetStringPtr()); | ||
| } |
There was a problem hiding this comment.
キャパがゼロになるのも仕様な認識です。
これも後追いでいいです。
|
✅ Build sakura 1.0.2374 completed (commit d81719325b by @beru) |
|
@berryzplus さん レビューありがとうございます。 |
CNativeW::SetString に NULL を指定した場合に wcslen に NULL を渡して落ちてしまう不具合を修正
@berryzplus さん、レビューお願いします。
PR の目的
CNativeW::SetString の引数に NULL が渡ってきた場合に wcslen にそのまま NULL を渡して落ちてしまう不具合を修正するのが目的です。
カテゴリ
PR の背景
実際に呼ばれるケースが存在するのかは不明ですが（実アプリでの再現手順は不明）起きうるかもしれないので対処を入れました。ユニットテストに落ちる事を確認する DEATH テストが入っていましたが、落ちる動作をわざわざ期待してその挙動を保つ必要も無いのかなと思います。
PR のメリット
CNativeW::SetString の引数に NULL が渡ってきた場合に落ちる事が無くなります。
PR のデメリット (トレードオフとかあれば)
CNativeW::SetString の引数の NULL チェックの分だけ処理コストが（わずかに）増えます。
PR の影響範囲
CNativeW::SetString の呼び出し箇所の動作に影響します。しかし落ちる動作を保つべきという事も無いと思います。
関連チケット
#1086