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

検索条件の文字列をエスケープする処理を関数化する #1132

Conversation

berryzplus
Copy link
Contributor

PR の目的

CGrepAgent::DoGrep関数を分かりやすくするために、関数内の重複処理に名前を付けて関数化します。処理に名前が付くことによって「何をしているか」が明確になり、分かりやすくなります。

カテゴリ

  • リファクタリング

PR の背景

CGrepAgent::DoGrep関数は、Grep検索とGrep置換の実処理を担当する部分です。

長年の機能追加により、処理内容がかなりぐちゃぐちゃになっており、
リファクタリングが必要な関数だと思っています。

サクラエディタのGrep機能は比較的信頼できる部類のツールですが、
Grep処理がシングルスレッドで行われる都合でパフォーマンスがよくありません。

処理のマルチスレッド化を行うにあたっては、
基本的な部分の整理が必要と考え、
まずは「見たら分かる」レベルの簡単なリファクタリングを提案してみることにしました。

PR のメリット

  • 検索条件の文字列リテラルをタイプ別設定に従ってエスケープする処理に名前が付きます。(EscapeStringLiteral)
  • 検索対象文字列と置換後文字列のエスケープ処理を共通化するので、今後エスケープ処理を変えたくなったときに、どちらかの変更をし忘れる事態を防ぐことができます。

PR のデメリット (トレードオフとかあれば)

  • 検索対象文字列と置換後文字列のエスケープ処理を共通化してしまうので、今後エスケープ処理を意図的に分けたくなったときに困る可能性があります。
    ※そういう趣旨の変更がレビューで通る事態は想像できませんが、将来どうするかは分からんので。

PR の影響範囲

  • アプリ(=サクラエディタ)の機能に影響はありません。
  • 改修箇所は、Grep検索およびGrep置換の「検索条件」と、Grep置換の「置換後」の表示方法に関連する部分です。処理内容は一切変えていないので変更前後比較をしても何も変わりません。

関連チケット

参考資料

@AppVeyorBot
Copy link

パラメータを出力対象ビューからタイプ別設定に変更。
@AppVeyorBot
Copy link

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

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

問題無いと思います。
動作確認はしていません…。

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
何か問題が見つかったら別PRで対処していきます。

@berryzplus berryzplus merged commit 48021e1 into sakura-editor:master Jan 11, 2020
@berryzplus berryzplus deleted the feature/extract_escape_string_literal branch January 11, 2020 08:41
cmemWork.AppendNativeData( cmemWork2 );
cmemWork.AppendString( L"\"\r\n" );
cmemWork = EscapeStringLiteral( type, *pcmGrepKey );
cmemMessage.AppendStringF( L"\"%s\"\r\n", cmemWork.GetStringPtr() );
Copy link
Contributor

Choose a reason for hiding this comment

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

cmemWork=>pcmGrepKey は、GUI、コマンドラインともに、普通に2048文字を超える検索に対応しているので、アウトですね。
cmemReplaceのほうも同様です。

Copy link
Contributor

Choose a reason for hiding this comment

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

//! バッファの最後にデータを追加する (フォーマット機能付き)
void CNativeW::AppendStringF(const wchar_t* pszData, ...)
{
wchar_t buf[2048];
// 整形
va_list v;
va_start(v, pszData);
int len = _vsnwprintf_s(buf, _countof(buf), _TRUNCATE, pszData, v);
int e = errno;
va_end(v);
if (len == -1) {
DEBUG_TRACE(L"AppendStringF error. errno = %d", e);
throw std::exception();
}
// 追加
this->AppendString(buf, len);
}

お、本当ですね。言われてみるとそういえばそうだった…感が…。

Copy link
Contributor

Choose a reason for hiding this comment

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

#1135 を作成しました。

@m-tmatma m-tmatma added this to the v2.4.0 milestone Apr 19, 2020
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…_escape_string_literal

検索条件の文字列をエスケープする処理を関数化する
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants