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

#1914 で確認した問題への対策を追加 #1966

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

beru
Copy link
Contributor

@beru beru commented Jul 15, 2024

PR対象

  • アプリ(サクラエディタ本体)

カテゴリ

  • 不具合修正

PR の背景

#1914 (comment)
#1914 (comment)

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

beru added 2 commits July 16, 2024 02:02
…に負の値が size_t 型のローカル変数に代入され、極端に大きい値が std::vector::resize メソッドに渡されてしまう問題への対策を追加 
…超過すると pMetrics->GetDxArray_AllHankaku() で取得する領域の範囲外参照が発生する問題への対策を追加
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jul 15, 2024
@beru beru self-assigned this Jul 15, 2024
Copy link

@AppVeyorBot
Copy link

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; //!< 半角用文字間隔配列
Copy link
Contributor

Choose a reason for hiding this comment

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

ここが static なのが気になってます。

Copy link
Contributor Author

@beru beru Jul 16, 2024

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 付きのローカル変数にしました。

Copy link
Contributor

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()
Copy link
Contributor

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 に入れたら辻褄あいそうに思いました。

Copy link
Contributor Author

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 感があります。。

Copy link
Contributor Author

@beru beru Jul 16, 2024

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 して値を設定する実装で。

Copy link
Contributor

Choose a reason for hiding this comment

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

伝わったようで良かったです。
細かいことなのでスルーでよいと思ってます。

@berryzplus
Copy link
Contributor

本文を2か所修正して 解決対象の issue が分かるようにしました。

@beru
Copy link
Contributor Author

beru commented Jul 18, 2024

(更新コードがやっつけですが)問題解決の為にいったんマージします。問題が見つかったら別PRで修正します。

#1914 (comment) に書いたプロポーショナルフォントだと列の幅が広すぎる問題は解消していませんが、そこはとりあえず放置します。

@beru beru merged commit fe4cc6d into sakura-editor:master Jul 18, 2024
24 checks passed
@beru beru deleted the fix_csv_mode branch July 18, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSVを開いた時の動作が重くなった(2.4.2リリース版)
3 participants