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

CGrepAgent::DoGrep のローカル変数初期化漏れ修正 #236

Merged
merged 1 commit into from
Jul 11, 2018

Conversation

yoshinrt
Copy link
Contributor

@yoshinrt yoshinrt commented Jul 8, 2018

#225 の発端となった初期化漏れの修正です.
「ラインモード貼付けを可能にする」off 時は bLineSelect = false であるべきなので,そのように初期化.

ちなみ初期化の方法は↓に合わせています.
(どちらも,ダイアログボックスのテキストボックスにクリップボードのデータを貼り付ける動作)

bool bLineSelect = false; // ラインモード貼り付けを行うかどうか
CNativeW cmemClip; // 置換後文字列のデータ(データを格納するだけで、ループ内ではこの形ではデータを扱いません)。
// クリップボードからのデータ貼り付けかどうか。
if( nPaste != 0 )
{
// クリップボードからデータを取得。
if ( !m_pCommanderView->MyGetClipboardData( cmemClip, &bColumnSelect, GetDllShareData().m_Common.m_sEdit.m_bEnableLineModePaste? &bLineSelect: NULL ) )

@yoshinrt
Copy link
Contributor Author

yoshinrt commented Jul 8, 2018

他の C4701 については↓以外は自分目線で問題なさそう,程度しか見ていませんので,#225 を残す・閉じる は他の方の判断におまかせしたいと思います.

d:\dds\sakura\sakura_core\cmd\cviewcommander.cpp(671): warning C4701: 初期化されていない可能性のあるローカル変数 'bSelLock' が使用されます [D:\DDS\sakura\sakura\sakura.vcxproj]

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.

PRありがとうございます。
LGTMです。

Grep置換で「クリップボードから貼り付け」にして動かしてみましたが動作にも影響していないようです。

@@ -231,7 +231,7 @@ DWORD CGrepAgent::DoGrep(
if( bGrepPaste ){
// ��`�E���C�����[�h�\��t���͖��T�|�[�g
bool bColmnSelect;
bool bLineSelect;
bool bLineSelect = false;
if( !pcViewDst->MyGetClipboardData( cmemReplace, &bColmnSelect, GetDllShareData().m_Common.m_sEdit.m_bEnableLineModePaste? &bLineSelect: NULL ) ){
Copy link
Contributor

@berryzplus berryzplus Jul 8, 2018

Choose a reason for hiding this comment

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

参考ですが、このbLineSelectは、SALで書くと _Out_writes_opt_ bool* bLineSelect に渡すパラメータです。平たく言うと「出力パラメータなので初期化しなくてもいい」です。

呼び出されるメソッドの中

bool CEditView::MyGetClipboardData( CNativeW& cmemBuf, bool* pbColumnSelect, bool* pbLineSelect /*= NULL*/ )
{
if(pbColumnSelect)
*pbColumnSelect = false;
if(pbLineSelect)
*pbLineSelect = false;

さらに呼び出されるメソッド

bool CClipboard::GetText(CNativeW* cmemBuf, bool* pbColumnSelect, bool* pbLineSelect, const CEol& cEol, UINT uGetFormat)
{
if( !m_bOpenResult ){
return false;
}
if( NULL != pbColumnSelect ){
*pbColumnSelect = false;
}
if( NULL != pbLineSelect ){
*pbLineSelect = false;
}
//矩形選択や行選択のデータがあれば取得
if( NULL != pbColumnSelect || NULL != pbLineSelect ){

個人的に、初期化漏れパラメータはすっごく気になるんですけど、こういうこともあるようです。

Copy link
Member

Choose a reason for hiding this comment

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

参考ですが、このbLineSelectは、SALで書くと Out_writes_opt bool* bLineSelect に渡すパラメータです。平たく言うと「出力パラメータなので初期化しなくてもいい」です。

GetDllShareData().m_Common.m_sEdit.m_bEnableLineModePaste が FALSE の場合は
&bLineSelect が渡されないので初期化されないと思います。

@m-tmatma m-tmatma added this to the next release milestone Jul 10, 2018
@m-tmatma m-tmatma added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jul 10, 2018
@m-tmatma
Copy link
Member

テスト方法を共有していただけますか?

@berryzplus
Copy link
Contributor

berryzplus commented Jul 10, 2018

GetDllShareData().m_Common.m_sEdit.m_bEnableLineModePaste が FALSE の場合は
&bLineSelect が渡されないので初期化されないと思います。

その通りですね。
Releaseビルドでテストしたので普通に通ったようです。

テスト方法を共有していただけますか?

	if( bGrepReplace ){ //←Grep置換ダイアログで「置換」
		if( bGrepPaste ){ //←Grep置換ダイアログで「クリップボードから貼り付ける」にチェック

GetDllShareData().m_Common.m_sEdit.m_bEnableLineModePaste の設定は
共通設定 > 編集タブ
「ラインモード貼り付けを可能にする」のON/OFFで切り替わる

クリップボードに入っているデータの種類によって動作を切り替えるかどうかの設定です。

  1. vs2017もしくはサクラエディタで、データを選択せずにコピーする。
    vs2017もしくはサクラエディタで、カーソルを行の途中(行頭でも行末でもない位置)において貼り付けるとカーソル行の手前にコピー時にカーソルがあった行の全体が貼り付けされる。通常モードではカーソル位置にデータが貼り付く。これがラインモード貼り付け。
  2. Grep置換ダイアログを開いて「クリップボードから貼り付ける」をチェックして「置換」を実行。
    ファイルが置換されてしまうので対象フォルダにはダミーデータを用意しておく。
  3. 設定に応じて「ラインモード貼り付け」の有効・無効が切り替わるか確認する。
    ラインモード有効なら検索一致行の前にコピーした行が挿入される。
    ラインモード無効なら検索一致文字列が置換される。

ぼくが確認したのは変更前後のリリース版の挙動。

テストしないといけないのは変更前後のデバッグ版の挙動。
デバッグ版だとラインモード無効時の挙動が変わるはず。
変更前はラインモード有効になり、変更後は設定通り無効になる。

デバッグ版だとアサーションで止まるかも、ということに書いてから気付いた。

@yoshinrt
Copy link
Contributor Author

@berryzplus さん,テスト方法の共有ありがとうございます.
デバッグビルドだと,アサーション発火しました.

ラインモード有効なら検索一致行の前にコピーした行が挿入される。

自分で直しといてなんですが,置換時におけるこの仕様って需要あるんでしょうか...
m_bEnableLineModePaste にかかわらずラインモード貼付けは無し,が自分的にはしっくりきます.

@kobake
Copy link
Member

kobake commented Jul 11, 2018

master 時でのアサーション発火、こちらでも確認。

本 PR 摘要後にはこれは出なくなっています。

Copy link
Member

@kobake kobake left a comment

Choose a reason for hiding this comment

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

対応内容については LGTM です

機能そのものの需要とか正当性とかについては別途議論ですかね~

@kobake kobake merged commit 7e668d2 into sakura-editor:master Jul 11, 2018
@yoshinrt yoshinrt deleted the fixup_dogrep_localvar_init branch July 11, 2018 11:25
@ds14050 ds14050 added the 🐛bug🦋 ■バグ修正(Something isn't working) label Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…var_init

CGrepAgent::DoGrep のローカル変数初期化漏れ修正
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.

5 participants