CEditView に残存するデッドコードを削除する #1496
Conversation
|
✅ Build sakura 1.0.3333 completed (commit 1571ee15c5 by @k-kagari) |
There was a problem hiding this comment.
このPRは4つの変更で構成されています。
- 未使用関数の削除
- 古いWindows SDKによるビルドを可能とするための独自構造体定義の削除
- 関数定義の変更（引数の削除はシグニチャの変更です。）
- 関数定義の変更により不要となった定数の削除
1と2は文句なしでマージ可能と思います。
3の変更が正しいことをレビューできる人材はこのプロジェクトにはいません。
4は3の変更が可能であれば文句なしでマージ可能と思います。
複雑な変更のレビューが不能であることの対策としての単体テスト導入であり、SonarCloud導入です。
「ナメるな！」とレビューに挑むのは自由ですが、
なんかあったら２ｃｈのアフォどもにdisられることになります。
ぼくはそれでも（≒なんかあってdisられても）よいですが、
他の方は「嫌」だと思っていると認識しとります。
よって、現状でレビュー不可と判断して、PRの分割を提案します。
sakura_core/view/CEditView_Ime.cpp
Outdated
| @@ -288,62 +287,20 @@ LRESULT CEditView::SetReconvertStruct(PRECONVERTSTRING pReconv, bool bUnicode, b | |||
| const void* pszReconv; | |||
| const void* pszInsBuffer; | |||
|
|
|||
| //UNICODE→UNICODE | |||
| if(bUnicode){ | |||
There was a problem hiding this comment.
削除しようとしている引数bUnicodeが使われています、
使われている変数の削除はロジック変更を伴うので、削除しようとしている変数が実質的に利用されていないのが分かるようにリファクタリングするのが先だと思いました。
sakura_core/view/CEditView_Ime.cpp
Outdated
| pszInsBuffer = pszCompInsStr; | ||
| } | ||
| //UNICODE→ANSI | ||
| else{ |
There was a problem hiding this comment.
PRの意図を汲むと「ここのelseはデッドコードなので削除しましょう」なんだと思います。
デッドコードじゃない部分も一緒に変えてしまってるので難しい変更に見えるのかな、と思いました。
この修正を、デッドコード削除のコミットと不要になったif文の削除に分けるのは大変ですかね。
この関数の変更内容が、デッドコード削除と不要になったif文の削除だけであることはわたしが確認しました。めんどうなのであらかじめレビューイ側で分かるようにしといたほうがよいとは思います。
There was a problem hiding this comment.
お時間をとらせて申し訳ありません。
この修正を、デッドコード削除のコミットと不要になったif文の削除に分けるのは大変ですかね。
修辞的な疑問だと理解しますので言うまでもないのですが、まったく大変ではないです。分けるという考えが自分の中にありませんでした。
私の意識の中ではインデントの変更で生じる巨大なdiffはレビュワーにとって意図が自明であると捉えていたからですが、この場では私一人が異質な文化の中にいるのだと思います。いつか同種の変更を提案する機会があればご指摘に従います。
There was a problem hiding this comment.
私の意識の中ではインデントの変更で生じる巨大なdiffはレビュワーにとって意図が自明であると捉えていたからですが、この場では私一人が異質な文化の中にいるのだと思います。いつか同種の変更を提案する機会があればご指摘に従います。
ステキな環境にいらっしゃるようですね。
ここのプロジェクトは、めちゃめちゃ賢い人でなくとも参加できるようにしていきたいと考えています。diffツールで巨大な差分があったら「よく分からない差分があるからレビューを諦める」なレベルの人でも参加して構わないと思っています。高いレベルの人が素人にも分かるよう説明することは可能なはずです。分かる人だけで話を進めちゃうのはなんかしがうかなぁ、と思っています（どう考えても無理そうな場合は諦めますが・・・）。
There was a problem hiding this comment.
文化云々と書いたのはレベルが高い低いの話ではなくて、参加メンバーが読み取るための文脈を持っているかどうかです。PRを読み取るのに必要な文脈の説明をすっとばした私に問題があるのであって、そのあたりの認識がたぶん噛み合ってないのでお返事しづらい感じです。
高いレベルの人が素人にも分かるよう説明することは可能なはずです。分かる人だけで話を進めちゃうのはなんかしがうかなぁ、と思っています（どう考えても無理そうな場合は諦めますが・・・）。
これは大事なことだと思います。
sakura_core/view/CEditView.h
Outdated
| @@ -354,7 +354,7 @@ class CEditView | |||
| void AddToCmdArr(const WCHAR* szCmd); | |||
| BOOL ChangeCurRegexp(bool bRedrawIfChanged= true); // 2002.01.16 hor 正規表現の検索パターンを必要に応じて更新する(ライブラリが使用できないときはFALSEを返す) | |||
| void SendStatusMessage( const WCHAR* msg ); // 2002.01.26 hor 検索／置換／ブックマーク検索時の状態をステータスバーに表示する | |||
| LRESULT SetReconvertStruct(PRECONVERTSTRING pReconv, bool bUnicode, bool bDocumentFeed = false); /* 再変換用構造体を設定する 2002.04.09 minfu */ | |||
| LRESULT SetReconvertStruct(PRECONVERTSTRING pReconv, bool bDocumentFeed); /* 再変換用構造体を設定する 2002.04.09 minfu */ | |||
There was a problem hiding this comment.
bDocumentFeed のデフォルト引数 false を削除しているのはミスですか？（笑）
この変更を行うと、フィードが不要な標準ケースに false を書いて回る必要が出てしまい、読む側は false ってなんだ？と宣言を確認したくなってしまうので良くないと思います。
対立する考え方として、呼出しの記述に差異が現れないインターフェース設計は誤りの原因になるので、省略可能なパラメータは可能な限り用いるべきではない、というものがあります。確かに一理ある考え方なんですけど、「省略できるものは省略可能にしてタイプ文字数を減らしましょう」がC++なので、やや違和感があります。
思想問題はどちらでも良いのですが、「説明されていない変更が意図した変更かどうか」を聞いておくのはレビュアーの責任だと思うので突っ込んでおきます。
There was a problem hiding this comment.
主題と無関係な変更が混じっている＆標準的なC++の書き方作法に照らして変更が正しくなさそうに見える、には同意なので、「デフォルト引数を辞める」は今回見送るのが妥当だと思います。
もっとも「好みに合わない」をaooriveしない理由とするつもりはぼくにもないのでこのまま進めてもよいような気もしています。
修正対象は IME を直接制御する古代方式なので、いずれバッサリ消えます。どうせそのうち消えるコードに対して一生懸命レビュー対応するのもアフォなので、細かいことにこだわる意味はないんじゃないか？という考え方もできると思います。
There was a problem hiding this comment.
おおむね「対立する考え方」に基づいて意図したものです。その是非はおいてもPR本文での言及があるべきでした。言及しなかったのは私の落ち度です。
| @@ -978,7 +978,7 @@ void CViewCommander::Command_Reconvert(void) | |||
| // Sizeはバッファ確保側が設定 | |||
| pReconv->dwSize = nSize; | |||
| pReconv->dwVersion = 0; | |||
| m_pCommanderView->SetReconvertStruct( pReconv, UNICODE_BOOL || bUseUnicodeATOK); | |||
| m_pCommanderView->SetReconvertStruct( pReconv, false ); | |||
There was a problem hiding this comment.
実害はなさそうですが、bUseUnicodeATOK も横並びで未使用となると思います。
bUseUnicodeATOKの削除をこのPRに含めるべきだとは思いませんが、使われ方を見る限りでは同時削除が分かりやすそうな雰囲気を感じました。この件、詳細は確認してないので聞き流してもらって結構です。
There was a problem hiding this comment.
これについては見落としでした。あえて別にPRするまでもないのではないか？とも思うので扱いに困るところではありますが…。
sakura_core/view/CEditView_Ime.cpp
Outdated
| DWORD dwOffset, dwLen; | ||
|
|
||
| //UNICODE→UNICODE | ||
| if(bUnicode){ |
There was a problem hiding this comment.
ここの変更もOKです。
実際にやってるのは、デッドコード削除と不要になったif文の削除、無駄な変数の前方宣言を代入式と統合する、の3点で、それ以外の変更は含まれてなさそうです。
kengoide
force-pushed
the
feature/eliminate-dead-code-ime
branch
from
6a46d85 to
9c39c53
Compare
|
bUnicode 引数の削除および UNICODE_BOOL 定数の削除については取り止めます。説明が足りていないPRを丁寧に読み解いていただいたにもかかわらず申し訳のない思いです。そもそも使われていないために害のないコードであることと、IMM依存から脱却したどこかの未来では消えているコードであることを踏まえ、不適切である/異論のある変更の取り扱いについてレビュワーの皆様の時間を今以上に費やす意義がないと考えるためです。 |
|
✅ Build sakura 1.0.3335 completed (commit b3823b512b by @k-kagari) |
|
ありがとうございました。マージいたします。
実際のところ何であったのかを確認してみました。 sakura/sakura_core/config/build_config.h Lines 29 to 34 in c08893c _UNICODEが定義されているか否かを表現する定数だった模様です。 |
引数にしていること自体が、設計的に誤りだったってことですね。 |
PR の目的
CEditView に残る使われていない古いコードの削除を目的とします。
このPRで削除を提案するのは以下の5点です。2点になりました。CEditView::SetReconvertStruct の bUnicode 引数CEditView::SetSelectionFromReonvert の bUnicode 引数。関数名のtypoも同時に直しています。#if WINVER < 0x040Aで囲われている）UNICODE_BOOL 定数カテゴリ
PR のメリット
PR のデメリット
PR の影響範囲
動作を変更しないはずのPRですが、再変換に関係するコードが変更対象に含まれるため、以下にテスト内容を記載しています。参照されていないコードの削除であるため、プログラムの動作には影響しません。
テスト内容