sakura icon indicating copy to clipboard operation
sakura copied to clipboard

CPPA::stdErrorにテストを導入したい

Open berryzplus opened this issue 3 years ago • 3 comments

(必須) やりたいこと(=実現したいこと)

#1722 で報告された CPPA::stdError のバグを修正したいです。

修正にあたっての課題事項がたくさんあるので issue 化します。

修正にあたっての課題

  • 該当コードは「外部ライブラリ(PPA.DLL)」に依存しているため、アプリの実動作でバグを確認することは困難または不可能です。 👉つまり、不具合の再現や修正の効果確認を行う方法がありません。
  • 該当コードは「外部ライブラリ(PPA.DLL)」に依存しているため、文字列を「SJISエンコードのバイトシーケンス」で扱う必要があります。現状のサクラエディタは「文字列とはワイド文字列のことである」的な方向に進んでいて、std::string を使うコードが書きづらくなっています。 👉つまり、std::string を扱えるように独自定義関数を機能拡張する必要があります。

(省略可) 解決手段の提案

  • [ ] 該当コード(CPPA::stdError)の単体テストを用意する。 アプリ実動作での確認が不可能なので、該当処理を単体で動かせるようにします。 実現すれば、不具合の再現や修正の効果確認を出来るようになります。
  • [x] strprintf を拡張してstd::string を扱えるようにする。(#1810) 固定長の char[] を多用する現状のコードに手を入れるには、CとC++をシームレスに繋ぐ独自関数が必要です。 実現すれば、元々の処理をあまり変えずに修正できるようになります。

berryzplus avatar Feb 27 '22 04:02 berryzplus

目的が変わったのでタイトル変えておきます。

報告されていた不具合は、バグじゃなさそう、ということが分かったため。 (ただし、IsBadStringPtrAの利用は「知る人ぞ知る」系の「最も危険な種類」のバグです。実害はないと思いますが。)

berryzplus avatar Mar 02 '22 10:03 berryzplus

ぶっちゃけると、そもそも関数名からして「誤っている」んですよね。

誤) CPPA::stdError 正) CPPA::ErrorProc

PPAが要求するのは ErrorProc であり stdError は呼出側で勝手に付けた名前です。 privateな関数名なので好きな名前を付けていいんですが、 stdErrorは誤解を招きやすいのでやめたほうが良いと思います。

  • std はCプログラマ的に「標準の~」を意味する接頭辞です。 定義しているものは明確な「非標準関数」(ユーザー定義関数ともいう)なので、不適切です。
  • stderrは「標準エラー出力(ストリーム)」を指します。 stdError をユーザー定義関数の名前とするのは誤解を招きやすく、不適切だと思います。 PPAから要求されている ErrorProc を作るならともかく、関数名から処理内容を推測できないのもイタいと思います。

名前付けの「センスがない」のは「罪じゃない」です。 気付いた人が突っ込んで、直していけばいいだけのことだと考えています。

berryzplus avatar Mar 11 '22 12:03 berryzplus

CPPA::stdErrorはプライベートなグローバル変数CPPA::m_CurInstanceに依存します。

https://github.com/sakura-editor/sakura/blob/e67c8ce03f808ae940689373a82e01d6bc066043/sakura_core/macro/CPPA.cpp#L333-L338

CPPA::m_CurInstanceはCPPAの実行コンテキストを保持する構造体です。 プライベートなグローバル変数ですので、クラス外から値を変更することはできません。 CPPA::m_CurInstanceの初期値はnullptrなので、上記コードをテストコードから呼び出すとAV例外が発生します。

テストしたいコードは339行目以降なので、CPPA::stdErrorをテストするにはCPPA::m_CurInstanceに有効なPPAコンテキストを設定するために仕組みが必須になります。

berryzplus avatar Mar 19 '22 07:03 berryzplus