Grep時に「ripgrepを使う」オプションを追加 #1211
Conversation
|
❌ Build sakura 1.0.2645 failed (commit fab97db8c1 by @7-rate) |
|
折角ですが「なんか嫌 😕 」です。 「サクラエディタのGrepは遅いので、他のツールを使います。」になってる気がします。 「サクラエディタのGrep」が遅いのを外部ツールが解決することはないと思います。 「サクラエディタのGrepダイアログを使って rg.exe を使えるようにしたい」であればニーズは理解しますし、そういうのはあってもいいと思っています。 まぁ、この形式の拡張をすると「なんで |
|
誤解があったかもしれません。 その手段として下記がある認識ですが、目的からすればいずれの手段でも問題ないと考えています。 ・既存のサクラエディタのGrep機能を改善する。 ・サクラエディタから高速Grep可能な外部ツールを叩き、その出力を得る また、rg.exeに拘っている理由はないです。 ｢なんで rg.exeだけやるんだ！｣についても、上と同じ内容です。 |
|
Releaseビルドが通らないのは後ほど修正します |
|
grep ライクなツールだと、pt や jvgrep は、複数エンコーディングに対応しているのが便利ですね。 |
|
✅ Build sakura 1.0.2646 completed (commit 1e48e21b8d by @7-rate) |
|
ビルドエラーの原因は、独自定義のカスタムint 型がリリースビルドではただのintになるからっぽいですね。 |
|
結局のところ CGrepAgent::GetFirstFilePath はフルパスを返すのでしょうか？ ファイル名を返すのでしょうか？
不本意ながらこの意見には理があります。 複数の grep 実装を取り替えることを視野に入れるなら、rg.exe とその他の exe のあいだの共通部分と可変部分を見極めて、共通部分をコードに、可変部分をデータにできると拡張とメンテナンスが容易になります。すべてをコードで表現する(「rg.exe特化実装を入れる」)と、実装者には最大限の自由と最適化の余地が与えられますが、後々の面倒を背負い込みます。 それ以前に sakura.exe による GREP 実装と rg.exe との関係もあります。 CGrepAgent::DoGrepRipgrep からコアとなる処理を抜き出して、すでに存在している CGrepAgent::DoGrepTree というインタフェースに適合する代替実装とするなら、細々とした雑事を再実装しなくて済んだと思うのですが、それではダメだったのでしょうか？ それに関連して気がついたことですが、rg.exe にそのまま渡している pcmGrepFolder の内容はセミコロン区切りのパスになることがあり、それはサクラエディタの独自仕様だと思います。dlg/CDlgGrep.cpp に「// 2011.11.24 Moca 複数フォルダ指定」というコメントがありました。rg.exe が受け取るオプションの仕様が不明ですが問題があるなら、分割処理が DoGrepTree の呼び出し側にありますので、DoGrepTree の代替実装であればこれも雑事として再実装を避けながら自動的に対応ができます。 |
pcmGrepFolderからの相対パスになります。関数コメント、関数名ともに合ってないですね。
少なくとも私は考えていません。 @ds14050 さんが仰ることを要約すると、
この仕様は見落としていました。ripgrepを使う場合は動かないですね...修正します。 |
|
つけが積み上がっている気がしますが最終的にそれは中の人と実装者が気にすることです。 倍以上速いのなら多少の瑕疵(文字化けとかジャンプとか)には目をつぶってでも取り敢えず使ってみたい人はいるのではないでしょうか。今日の出来事ですが、100 MiB 以上あるデバッグ版の sakura.exe の中を入念に時間をかけて GREP してしまう今の sakura.exe は多少残念ですから、それが避けられるだけでも使ってみたくなるというものです。 比較すると些細なことで書き漏らしてしまいましたが、this ポインタや private メンバへのアクセスを要求しない関数をメンバにするのはおすすめできません。CGrepAgent の public メンバに目をやった後では本当に些細なことですが……。 |
| @@ -218,6 +218,7 @@ BEGIN | |||
| DEFPUSHBUTTON "検索(&F)", IDOK, 288, 125, 49, 14, WS_GROUP | |||
| PUSHBUTTON "キャンセル(&X)", IDCANCEL, 288, 149, 49, 14 | |||
| PUSHBUTTON "ヘルプ(&H)", IDC_BUTTON_HELP, 288, 174, 49,14 | |||
| CONTROL "ripgrepを使う", IDC_CHK_USERIPGREP, "Button", BS_AUTOCHECKBOX | WS_TABSTOP, 155, 220, 60, 10 | |||
There was a problem hiding this comment.
リソースを更新する際は英語版のファイルも同等の項目を用意するように更新お願いします。
There was a problem hiding this comment.
一応、見解を出しておきます。NGです。
最近読んだ本に印象的なフレーズが載っていました。
「コピーアンドペーストは設計ミスである」
サクラエディタには、元々「うんこ実装」が多いんですが、GitHubに移って「ちょっと真面目にレビューしてみようぜ！」と言ってる中で明らかなうんこを入れるのはどうかという話をします。
誤解している人がいる気がするので「うんこ実装」の定義を解説します。
- うんこ実装とは
- これはプログラミングのプロが使う業界用語です。
- 要件を実現できるように書いた最初のコードのことを言います。
- 通常、要求された要件のすべてを取り込んだ動く実装になっています。
- プロの世界では、うんこを出してからが本当の設計になります。
- 一般的に「製品」とは、うんこを肥しにして育て上げた「果実」のことを指します。
サクラエディタが「製品」なのかどうかは知りません。
フリーソフトなので、まるまるうんこで何も問題ないのかも知れません。（ぼくは嫌だっ！
「うんこ実装」の定義が分かれば、「明らかなうんこは嫌だな」と言ったり「うんこでも構わん」と言ったりする感覚が少しわかるかも知れないと思ってちょっと書いてみました。
| if( NULL == pcViewDst->GetDocument()->m_cDocEditor.m_pcOpeBlk ){ // 操作ブロック | ||
| pcViewDst->GetDocument()->m_cDocEditor.m_pcOpeBlk = new COpeBlk; | ||
| pcViewDst->GetDocument()->m_cDocEditor.m_nOpeBlkRedawCount = 0; | ||
| } |
There was a problem hiding this comment.
この辺りの処理ですが、本質的に不要という見解です。
元コード CGrepAgent::DoGrep には呼出元が3か所ありまして、そのうちの1つ、CNormalProcess::InitializeProcessからの呼出でのみ必要な処理です。他2か所はCViewCommanderの共通処理で相当する呼出しを行っているので不要になります。
CNormalProcess::InitializeProcessでの呼出しが必要な理由はここで呼び出さないとSTDOUTへの出力モードで困るからです。STDOUTへの出力を行うモードでは、この辺の処理を行う必要がありません。（実行後、選択の余地なくプログラムが終了するから。
元が悪いっちゃそれまでなんですが、対応は必要だと思います。
|
ではこのPRとしてはクローズします。 |
|
@7-rate さん本人や潜在的なコントリビュータを萎縮させかねないクローズのされ方に見えたので、異なる見解を述べておきます。#1211 (comment) に対する見解です。 上記のコメントについて、現在のサクラエディタで機能していて問題がない、けれど改善の余地があると一人の開発者が考えている部分について、@7-rate さんが運悪くコピペ改変により再利用してしまったために、対応が求められてしまったのだと理解しました。 この問題認識は共有されているものでしょうか。取り除いて問題がないか、誰が確かめるのでしょうか。複数の呼び出し元が関係する処理をどのように取り除くのか、誰が決めるのでしょうか。前例踏襲から踏み込んで逸脱すべきなのは @7-rate さんなのでしょうか。それをこの PR でするのでしょうか。すべてネガティブです。 個人的な問題認識に基づくお門違いの修正要求が、ただの PR 潰しとして働いたのではないかと懸念しています。 私はすでに rg.exe をダウンロードしていて、使いたいと思っています。とりあえず動くものを取り込んで、それから細かい修正を積み上げて磨き上げるのでいいと思っていました。 「なんか嫌」という感想や、文脈を伴わない格言のコピペなど役には立ちません。他人に自分のご機嫌伺いをさせるのではなく、採用に至るまでにクリアすべき課題を明示し、具体的なアドバイスを与えて然るべきです。具体的であればこそ、その修正要求はお門違いではないかという意見も出せます。ゴールに向かう建設的、発展的なやりとりになります。 |
|
＞ゴールに向かう建設的、発展的なやりとりになります。 実装が「う〇こ」なのを綺麗にしたい（リファクタリング的な）というのはよくわかります。 そのなかで、実装を綺麗にしたいんじゃなくて、ripgrepを使いたい（もしくは利用するgrepツールを自由にAddInする実装を実現させたい）ならば、特に止める要素はないかなと思いました。 ただ開発チーム総意の結論であれば今まで通り私の口をはさむところではないかなと。 |
「このPRとしては」から、ぼくの意図はちゃんと伝わってそうだと思ってます。 #1210 で 現状、サクラエディタには検索に使えるエンジンが4種類あります（正規表現、単語検索、普通の検索、migemoのiSearch）。Grepに使えるのはこのうち3種類です。「Grep専用に5種類目を増やしたらいい」という認識です。 リファクタリングにより5種類目を追加できるようになるまでの手数は100手くらいです。コピー元コードには、少なくともPR 100本分の改善の余地があると考えています。 |
|
100手の100という数字の根拠は分かりかねますが、 |
|
遺恨がなかったのなら何よりです。@7-rate さんがどこから何を読み取ることができていたのか、私には未だにさっぱりですが……。 |
PR の目的
#857 でGrep処理に高速化できる余地があることを報告しました。
高速化の手段として@beruさんから教えていただいたripgrepをsakuraエディタに組み込んでみました。
sakura.exeと同じディレクトリにrg.exeを用意し、Grepダイアログで「ripgrepを使う」にチェックするとripgrepにてGrepを行います。
カテゴリ
PR の背景
#857 への対応。
PR のメリット
ripgrepを使う場合、既存のGrep機能より圧倒的に高速です。
今回対応したripgrepを使う場合において、#857と同様のベンチマークを行いました。
このように、60%～80%ほど高速化しました。
PR のデメリット (トレードオフとかあれば)
・ripgrepを使う場合は別途rg.exeのダウンロードが必要になります。
→インストーラーに同梱してもいいかもしれません。(msvc版のバイナリであれば5MB程度)
また、PRの変更内容だと以下の制約があります。
・Grep結果からダイレクトタグジャンプを行う時に、ripgrepの出力フォーマットの関係上、桁位置まで合わない。
・Grep対象のディレクトリにとあるファイルはUTF-8、別のファイルはSJISだったり、文字エンコードが混在しているとGrep結果の出力が文字化けする。
PR の影響範囲
既存の機能への影響はありません。
関連チケット
#857
→このPRの発端。
#809
→ripgrepはデフォルトで.gitignore等のignoreファイルを解釈して検索対象を絞っています。
このPRがマージされればgit grepの対応は不要になるかもしれません。
#424
→ripgrepはデフォルトでバイナリファイルを除外して検索します。
このPRがマージされればバイナリファイルを除外するオプションの対応は不要になるかもしれません。
参考資料
ripgrep