-
Notifications
You must be signed in to change notification settings - Fork 163
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
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
686110d
CNative::Clear のテストを追加
m-tmatma 4ee872b
CNative::Clear の実装を改善
m-tmatma 7c127d7
CStrictInteger.h のヘッダのインクルードの依存関係の不足を修正
m-tmatma c75432c
CNativeW::Clear のテストにコメントを追加、テストを追加
m-tmatma 664adce
ヘッダを追加
m-tmatma 25510c3
コメント追加
m-tmatma File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,5 +5,5 @@ | |
//! 空っぽにする | ||
void CNative::Clear() | ||
{ | ||
this->SetRawData("",0); | ||
this->_GetMemory()->_SetRawLength(0); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/*! @file */ | ||
/* | ||
Copyright (C) 2018-2019 Sakura Editor Organization | ||
|
||
This software is provided 'as-is', without any express or implied | ||
warranty. In no event will the authors be held liable for any damages | ||
arising from the use of this software. | ||
|
||
Permission is granted to anyone to use this software for any purpose, | ||
including commercial applications, and to alter it and redistribute it | ||
freely, subject to the following restrictions: | ||
|
||
1. The origin of this software must not be misrepresented; | ||
you must not claim that you wrote the original software. | ||
If you use this software in a product, an acknowledgment | ||
in the product documentation would be appreciated but is | ||
not required. | ||
|
||
2. Altered source versions must be plainly marked as such, | ||
and must not be misrepresented as being the original software. | ||
|
||
3. This notice may not be removed or altered from any source | ||
distribution. | ||
*/ | ||
#include <gtest/gtest.h> | ||
#include "mem/CNativeW.h" | ||
#include "mem/CNativeA.h" | ||
|
||
TEST(CNativeW, Clear) | ||
{ | ||
CNativeW stringW; | ||
stringW.AppendString(L"abc"); | ||
|
||
/* Clear() 前にバッファサイズを取得する */ | ||
auto orgCapacity = stringW.capacity(); | ||
|
||
/* Clear() 前にデータサイズを取得する */ | ||
auto orgLength = stringW.GetStringLength(); | ||
EXPECT_EQ(orgLength, 3); | ||
|
||
stringW.Clear(); | ||
|
||
/* Clear() 後にバッファサイズを取得する */ | ||
auto newCapacity = stringW.capacity(); | ||
|
||
/* Clear() 後にデータサイズを取得する */ | ||
auto newLength = stringW.GetStringLength(); | ||
|
||
/* Clear() 後にバッファサイズが変わっていないのを確認する */ | ||
EXPECT_EQ(orgCapacity, newCapacity); | ||
|
||
/* Clear() 後にデータが空なのを確認する */ | ||
EXPECT_EQ(newLength, 0); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
#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()の処理を変える必要はないと思います。
「この処理は安全です!」というコメントを入れとく意味はあると思います。
そういうコメントがあっても「ほんとに?」と思ってしまうのがプログラマのサガではありますが。
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.
データサイズをクリアすればよく、データのバッファを解放する
必要はないと思っています。
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.
単にCNativeTのデータをクリアする・空っぽにする
と言った場合、その詳細には「余分なメモリの開放」が含まれると思います。
もっと高級言語なら実際のバッファがどうなってるかなんて意識しませんが、
c++ではそのあたりの詳細も考えるべきことの一つになります。
CMemoryのメモリ再確保はサイズ拡大の場合だけになっています。
CNative::Clear()の実装から「余分なメモリの開放」を外すと
メモリ解放するタイミングがデストラクト時だけになってしまいます。
なので、外すにしても「余分なメモリの開放」を行う関数を用意する必要があると思います。
どちらかというと、「余分なメモリの開放」が不要であればこの関数を呼ばない、で切り抜けるのがよいと思います。