-
Notifications
You must be signed in to change notification settings - Fork 167
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
#1914 で確認した問題への対策を追加 #1966
#1914 で確認した問題への対策を追加 #1966
Conversation
…に負の値が size_t 型のローカル変数に代入され、極端に大きい値が std::vector::resize メソッドに渡されてしまう問題への対策を追加
…超過すると pMetrics->GetDxArray_AllHankaku() で取得する領域の範囲外参照が発生する問題への対策を追加
Quality Gate passedIssues Measures |
✅ Build sakura 1.0.4362 completed (commit bf63a184a0 by @beru) |
@@ -90,6 +90,14 @@ void CFigure_Comma::DispSpace(CGraphics& gr, DispPos* pDispPos, CEditView* pcVie | |||
if (szViewString.length() < nTabDispWidth) { | |||
szViewString.append(nTabDispWidth - szViewString.length(), L' '); | |||
} | |||
const INT* lpDx; | |||
if( szViewString.length() > 64 ) { | |||
static std::vector<int> anHankakuDx; //!< 半角用文字間隔配列 |
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.
ここが static なのが気になってます。
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.
このコンテナ変数を static なローカル変数にしているのは、変数の生存スコープを伸ばしたいからですが、どうして伸ばしたいのかというと呼び出しの度にメモリ確保と解放がされるのを回避する為です。ただそのコストがそんなに大きいのか?というと計測はしていません。あと static 変数を使うとリエントラントではなくなりますがまぁマルチスレッドは(ここらへんでは)あまり使われていないと思うので問題ないかなと。。
resize時のメモリ確保ですが、内部的にreserveしているサイズを拡張する必要が有る場合にはメモリの再確保が行われると思いますが、既に確保しているサイズ内に収まるのであればメモリの再確保は行われない実装になっている事を期待しています。VS2022のSTLのソースを読んだわけではないので憶測ですが。
当初は CFigure_Comma
クラスのメンバ変数にしようとしたのですが、この (override している) DispSpace
メソッドには const 指定が付いているので(mutable指定が無いメンバ変数の更新が出来ないので)諦めて static 付きのローカル変数にしました。
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.
キャッシュしたDX配列を更新するタイミングがないなぁ、と言う。
大きな問題でないと思います。
@@ -98,7 +106,7 @@ void CFigure_Comma::DispSpace(CGraphics& gr, DispPos* pDispPos, CEditView* pcVie | |||
&rcClip2, | |||
szViewString.c_str(), | |||
static_cast<UINT>(szViewString.length()), | |||
pMetrics->GetDxArray_AllHankaku() |
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.
このメソッドが返す配列のサイズが指定できないのは問題ないんでしょうか?
const int* GetDxArray_AllHankaku(int required = 64) const;
として 93-100行目の実装を GetDxArray_AllHankaku に入れたら辻褄あいそうに思いました。
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.
CTextMetrics::GetDxArray_AllHankaku
メソッドの実装ですが link std::array<int, 64>
型のメンバ変数 m_anHankakuDx
が持つメモリ領域の先頭のアドレスを返す実装になっています。固定長配列なので返す配列のサイズ指定が出来ないというのが単純な答えになります。
なおこのメソッドも const 指定が付いているのでメンバ変数を更新する事は (mutable指定しない限りは) 出来ません。
メソッド内部で static なコンテナのローカル変数を持って、サイズ指定が 64 を超過する場合はそのコンテナのローカル変数をresizeしてその領域のアドレスを返すような実装にすることは可能です。このPRで CFigure_Comma::DispSpace
メソッドに追加した実装内容を CTextMetrics::GetDxArray_AllHankaku
に移すような感じですね。ただ CTextMetrics::GetDxArray_AllHankaku
はヘッダ実装なのであまり複雑な記述にはしたくない気もします。
まぁこのRRで行った対処はかなりやっつけというか ad hoc 感があります。。
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.
思ったんですが CTextMetrics::m_anHankakuDx
の型を std::array<int, 64>
から std::vector<int>
に変える方が対処としてまともかもしれないですね。そもそも64要素の固定長配列だと要素数が不足しているという問題なので動的配列にするのが自然に思えてきました。
欲しい内部領域のサイズは引数で指定して、コンテナの要素数より大きい場合にはコンテナを resize して値を設定する実装で。
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.
伝わったようで良かったです。
細かいことなのでスルーでよいと思ってます。
本文を2か所修正して 解決対象の issue が分かるようにしました。 |
(更新コードがやっつけですが)問題解決の為にいったんマージします。問題が見つかったら別PRで修正します。 #1914 (comment) に書いたプロポーショナルフォントだと列の幅が広すぎる問題は解消していませんが、そこはとりあえず放置します。 |
PR対象
カテゴリ
PR の背景
#1914 (comment)
#1914 (comment)
仕様・動作説明
PR の影響範囲
テスト内容
関連 issue, PR
参考資料