ビルド時警告の削減：operator new/delete 部分 #51

kobake · 2018-06-03T15:42:22Z

operator new/delete の warning C9595 対策

build_config.h で inline 定義されていた operator new/delete の定義を build_config.cpp 側に移した。
build_config.cpp のエンコーディングを UTF-8 にする都合で、文字列定数で半角カナを使うのはやめてASCII文字列に差し替え。

関連PR

ビルド時警告の削減ビルド時警告の削減 #48

operator new/delete 部分についてはちょっと処理がややこしいので原作者 (kobake) のほうで対応。

build_config.h で inline 定義されていた operator new/delete の定義を build_config.cpp 側に移した。 build_config.cpp のエンコーディングを UTF-8 にする都合で、文字列定数で半角カナを使うのはやめてASCII文字列に差し替え。

m-tmatma · 2018-06-03T21:59:03Z

sakura_core/config/build_config.cpp

+
+void* operator new[](size_t nSize)
+{
+	void* p = ::malloc(nSize);


p に対して NULL チェックしないとメモリ確保に失敗したら落ちます。

20 行目も同様です。
_fill_new_memory で　NULL チェックするのでもいいです。

まあ、デバッグコードですけど

VC++のmallocはnewMode設定によってstd::bad_alloc例外を投げます。

malloc

_set_new_mode

mallocが例外を投げる想定ならNULLチェックは不要です。

_fill_new_memoryではNULLチェックしたほうが良いです。

--
書くべき場所が違った！
と気づいたので適切な場所に転記しますｍ（＿＿）ｍ

malloc の NULL チェックについてはその通りなのですが、元々の V1 のコードでは malloc の NULL チェックはまるで行われていなかったので、挙動としてはそのまま踏襲してあります。

ただ、（デバッグビルドに限りますが）このメモリ確保ロジックを build_config.cpp に集約させることによって、プログラム各地に存在するメモリ確保処理についてのエラー対策を全て build_config.cpp に処理を追加するだけの手間で注入することができます。

本PRについてはあくまでも既存機構の Warning 除去が主目的ですので、NULL チェック自体は別件で処理したい気持ちです。

メモリ確保時の NULL チェック時の処理（ここをどうするべきかは議論の余地あり）を注入しやすい場所があるという認識は伝わったかと思いますので、今後の話として良い感じの処理を埋め込める土台として活用できればと考えています。

いくつか選択肢はあります。

スタックトレースや使用メモリ状況等をログ出力した上でエラーダイアログを表示して強制終了

メモリマップトファイル等の機構を利用して一時的に凌ぎ（凌げるかな？）、ダイアログを表示してユーザに注意を促す

どちらにしろ簡単な変更ではないので、まずは別件の Issue 等で議論を行い対応を検討するのが良いところかと思います。

追記しておくと、今回の operator new/delete の書き換えの有無にかかわらず、プログラムの至る所でメモリ確保の NULL チェックは入っていないというのが現状です。

意図通りのコメントが書けないのは
ぼくがGitHubの使い方を分かっていないせいか、win8.1のIE11がダメなせいか。

趣旨了解しました。
警告削減のために既存コードをまるごとcppに退避させる、という内容ならばOKです。

26行目の #if ブロックはもう要らないんじゃ・・・
というのは別の機会に指摘するようにします。←書いてるやん

26行目の #if ブロックはもう要らないんじゃ・・・
というのは別の機会に指摘するようにします。←書いてるやん

これは、、たぶんいらないですね。
ただこのあたりの挙動は別件としてしっかり精査してから判断させてください。（ここの #if ブロック内容は自分以外の方が挿入されたものなので、いまいち挙動を把握しきれてないです）

既に閉じてしまっていますが参考情報を追記します。

sakura/sakura_core/config/build_config.cpp

Lines 25 to 31 in 6f23813

#ifdef _MSC_VER

#if _MSC_VER == 1500

_Ret_bytecap_(_Size) // for VS2008 Debug mode

#endif

#endif

void* operator new[](size_t nSize)

ぼくが書いた「#ifブロック要らない」は、中身をそのまま記述したらいい、という意味です。
_Ret_bytecap_(_Size) という書き方はメタプログラミングの一種です。
Microsoftが提唱する SAL というもので、最近のwindowsSDKはこの書き方になってるはずです。
たしかvista以降は一律適用。msdnの関数定義もこの書き方になってますよね。
→ WinMain@msdn _In_ というのがSALです。

SALにはバージョンがいくつかあるようです。
個人的には、windows SDK8.1のものがシンプルで好きです。
メリットはmsdn英語サイトの解説を見てください。
デメリットはvc++以外のコンパイラ向けに空defineを用意してやる必要があることです。
昨年末くらいに試した限りではMinGWに付属のWindowsAPIヘッダはSALに対応していませんでした。
CMakeに空defineを生成する機能とかプラグインとかあったらいいな、とか思ってみたり。

情報ありがとうございます。あとで調べてみます！

This reverts commit db5b838. sakura-editor#48 sakura-editor#51

kobake · 2018-06-04T01:17:41Z

@m-tmatma さんのほうでも問題なければマージお願いします

This reverts commit db5b838. sakura-editor#48 sakura-editor#51

sakura-editor#48 sakura-editor#51

m-tmatma

そもそもメモリ確保の失敗した時の件は、
このチケットのスコープではありませんでした。

berryzplus · 2018-06-04T05:40:47Z

はぅ、衝撃の事実。

sakura/sakura_core/config/build_config.cpp

Lines 25 to 31 in 6f23813

    
           #ifdef _MSC_VER 
        
           #if _MSC_VER == 1500 
        
           _Ret_bytecap_(_Size)	// for VS2008 Debug mode 
        
           #endif 
        
           #endif 
        
           void* operator new[](size_t nSize)

これが SAL だとすると書き方間違ってますね。（＝SALとして機能してない）

_Ret_bytecap_(nSize)
void* operator new[](size_t nSize)

こういうコードであれば「戻り値はnSizeバイト分のメモリ領域である」という意味の注釈になります。cl.exe /analyze したときに分析エラーになるだけで実行コードには影響を与えません。バグではないです。書いてあるコードが意味をなしていなかった、というだけです。（それもいいのか微妙...

operator newの戻り値注釈は、厳密にはもっと複雑です。
例外時は値を返さない仕様があったりするので…
書くならちゃんと書いたほうがいいのかも知れません。

sakura-editor#48 sakura-editor#51

…ew-delete-warnings ビルド時警告の削減：operator new/delete 部分

kobake mentioned this pull request Jun 3, 2018

ビルド時警告の削減 #48

Merged

m-tmatma requested changes Jun 3, 2018

View reviewed changes

berryzplus approved these changes Jun 4, 2018

View reviewed changes

berryzplus added a commit to berryzplus/sakura-editor that referenced this pull request Jun 4, 2018

ビルド時警告の削減 #48 (rebase)

3a1e21a

sakura-editor#48 sakura-editor#51

m-tmatma approved these changes Jun 4, 2018

View reviewed changes

m-tmatma merged commit 6f23813 into master Jun 4, 2018

kobake deleted the fix-operator-new-delete-warnings branch June 4, 2018 03:37

m-tmatma added this to the next release milestone Jun 4, 2018

berryzplus added a commit to berryzplus/sakura-editor that referenced this pull request Jun 4, 2018

ビルド時警告の削減 #48 (rebase2)

5246ee3

sakura-editor#48 sakura-editor#51

m-tmatma mentioned this pull request Jun 11, 2018

Source-Code annotation language (SAL) の記述が正しいか確認する #102

Closed

This was referenced Aug 30, 2018

new/deleteのオーバーロードがGCCに怒られる対応 #381

Closed

new/deleteのオーバーロードに対するGCC警告に対応する #382

Closed

KENCHjp added the CI appveyor など CI 関連【ChangeLog除外】 label Dec 5, 2018

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#51 from sakura-editor/fix-operator-n…

6887952

…ew-delete-warnings ビルド時警告の削減：operator new/delete 部分

berryzplus mentioned this pull request Sep 21, 2019

デバッグにVC++のCランタイム機能を活用する #1051

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ビルド時警告の削減：operator new/delete 部分 #51

ビルド時警告の削減：operator new/delete 部分 #51

kobake commented Jun 3, 2018

m-tmatma Jun 3, 2018

m-tmatma Jun 3, 2018

m-tmatma Jun 3, 2018

berryzplus Jun 4, 2018 •

edited

Loading

kobake Jun 4, 2018

kobake Jun 4, 2018

berryzplus Jun 4, 2018

kobake Jun 4, 2018

berryzplus Jun 4, 2018

kobake Jun 4, 2018

kobake commented Jun 4, 2018

m-tmatma left a comment

berryzplus commented Jun 4, 2018 •

edited

Loading

	#ifdef _MSC_VER
	#if _MSC_VER == 1500
	_Ret_bytecap_(_Size) // for VS2008 Debug mode
	#endif
	#endif

	void* operator new[](size_t nSize)

ビルド時警告の削減：operator new/delete 部分 #51

ビルド時警告の削減：operator new/delete 部分 #51

Conversation

kobake commented Jun 3, 2018

operator new/delete の warning C9595 対策

関連PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobake commented Jun 4, 2018

m-tmatma left a comment

Choose a reason for hiding this comment

berryzplus commented Jun 4, 2018 • edited Loading

berryzplus Jun 4, 2018 •

edited

Loading

berryzplus commented Jun 4, 2018 •

edited

Loading