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

CNative::Clear の実装を改善 #777

Merged
merged 6 commits into from
Feb 2, 2019

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Jan 24, 2019

@m-tmatma
Copy link
Member Author

#776 (comment) を分離して対応

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

実際にメモリ解放をしたいという事は無いでしょうか?
もしメモリ解放をしないように変えるなら、std::basic_string::shrink_to_fit 的なメソッドを追加するのも良いかもしれないと思いました。

https://en.cppreference.com/w/cpp/string/basic_string/clear
https://en.cppreference.com/w/cpp/string/basic_string/shrink_to_fit

なおに CNative::Clear がどういう使われ方をしているかの確認はまだしていないです。
別にメモリ解放しなくても問題無い使われ方かもしれないですね。

@m-tmatma
Copy link
Member Author

mingw でのビルドエラーは全然調べてないけど
なんかのヘッダのインクルードが足りないとかそういう系の問題ぽい

@beru
Copy link
Contributor

beru commented Jan 25, 2019

ローカルの mingw32 だとちゃんとビルド出来ました。何で AppVeyor では失敗するのか謎ですが、
CStringPoint.h ファイルの先頭でも primitive.h を #include すると解消しますか?モグラ叩きになってしまうかな。。

@@ -5,5 +5,5 @@
//! 空っぽにする
void CNative::Clear()
{
this->SetRawData("",0);
this->_GetMemory()->_SetRawLength(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#776 (comment) の指摘は、
this->SetRawData("",0); が「意図通りに動くか不安」という意味でした。

sizeof("") は 1byte になります。
0byte のメモリを確保したところに 1byte のデータをコピーして大丈夫なんでしたっけ?という不安です。

いまコードを追って確認してみた感じ、大丈夫そうでした。

SetRawDataは一度内部バッファを開放したあと、
引数で指定されたサイズ + 2を算出し、
確保サイズが8バイト単位になるように調整してからallocします。
指定サイズが 0 だった場合は 8byte 確保されます。

(0 + 2 + 7) & (~7)
= 9 & (~7)
= 0x09 & (0xF8) ← 2進表記で 00001001 & 11111000
= 0x08

+2 は wchar_t のNUL終端を意識した処理、
8バイト整列はCPUのフェッチ単位を意識した処理と思われます。
(もしそうなら確保サイズだけ気にしても意味無いんですがw)

なので、CNativeT::Clear()の処理を変える必要はないと思います。
「この処理は安全です!」というコメントを入れとく意味はあると思います。
そういうコメントがあっても「ほんとに?」と思ってしまうのがプログラマのサガではありますが。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetRawDataは一度内部バッファを開放したあと、
引数で指定されたサイズ + 2を算出し、
確保サイズが8バイト単位になるように調整してからallocします。
指定サイズが 0 だった場合は 8byte 確保されます。

データサイズをクリアすればよく、データのバッファを解放する
必要はないと思っています。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

単にCNativeTのデータをクリアする・空っぽにする

と言った場合、その詳細には「余分なメモリの開放」が含まれると思います。
もっと高級言語なら実際のバッファがどうなってるかなんて意識しませんが、
c++ではそのあたりの詳細も考えるべきことの一つになります。

CMemoryのメモリ再確保はサイズ拡大の場合だけになっています。

CNative::Clear()の実装から「余分なメモリの開放」を外すと
メモリ解放するタイミングがデストラクト時だけになってしまいます。

なので、外すにしても「余分なメモリの開放」を行う関数を用意する必要があると思います。

どちらかというと、「余分なメモリの開放」が不要であればこの関数を呼ばない、で切り抜けるのがよいと思います。

@m-tmatma
Copy link
Member Author

実際にメモリ解放をしたいという事は無いでしょうか?
もしメモリ解放をしないように変えるなら、std::basic_string::shrink_to_fit 的なメソッドを追加するのも良いかもしれないと思いました。

現状では CNativeW::Clear はローカル変数のインスタンスに対して、呼び出しているみたいです。
でも、既存機能に影響を与えないように、したほうがいいのかもしれないですね。

@m-tmatma
Copy link
Member Author

mingw でのビルドエラーは全然調べてないけど
なんかのヘッダのインクルードが足りないとかそういう系の問題ぽい

ビルドエラーは修正しました。 664adce

@m-tmatma
Copy link
Member Author

実際にメモリ解放をしたいという事は無いでしょうか?
もしメモリ解放をしないように変えるなら、std::basic_string::shrink_to_fit 的なメソッドを追加するのも良いかもしれないと思いました。

#778 を作成しました。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

問題ないと思います。

経緯はこの辺。
#778 (comment)

テストコードが入ることに大きなメリットがあると思うので、このまま入れてよいと思います。

@m-tmatma
Copy link
Member Author

m-tmatma commented Feb 2, 2019

説明欄を修正

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.

3 participants