CNativeW の static 関数に対するテストを追加する #1542
Conversation
|
✅ Build sakura 1.0.3450 completed (commit 4e7da81bf7 by @k-kagari) |
|
Code Smells に対応するまで Draft にしておきます。 |
|
✅ Build sakura 1.0.3451 completed (commit 0dec8bd846 by @k-kagari) |
kengoide
force-pushed
the
feature/more-tests-for-cnativew
branch
from
9f8ec63
to
2222e9c
Compare
|
✅ Build sakura 1.0.3452 completed (commit 23d3d49be0 by @k-kagari) |
|
すぐレビューできなくてすみません。 CodeFactorの警告がまともなのを久々に見た気がします。
|
|
✅ Build sakura 1.0.3453 completed (commit ac0da5c255 by @k-kagari) |
|
Kudos, SonarCloud Quality Gate passed! |
|
以下の修正が本題と関係しないように見えましたが、ついでに行うリファクタリングでしょうか。 sakura_core/CRegexKeyword.h sakura_core/recent/SShare_History.h |
CNativeW.h で charset/charcode.h と env/DLLSHAREDATA.h を #include すると発生するコンパイルエラーの修正でした。 |
| #include "debug/Debug2.h" //assert | ||
| #include "env/DLLSHAREDATA.h" |
There was a problem hiding this comment.
CNativeWはDLLSHAREDATAの定義に依存する、という意味の変更なので、若干気になっています。
方向的には逆ですよね。（暗黙的に依存していたのを解消させようとしてる）
| static CKetaXInt GetKetaOfChar( const wchar_t* pData, int nDataLen, int nIdx ); //!< 指定した位置の文字が半角何個分かを返す | ||
| static CHabaXInt GetHabaOfChar( const wchar_t* pData, int nDataLen, int nIdx, | ||
| CCharWidthCache& cache = GetCharWidthCache(), | ||
| bool bEnableExtEol = GetDllShareData().m_Common.m_sEdit.m_bEnableExtEol ); |
There was a problem hiding this comment.
個人的には cache と bEnableExtEol の定義順を逆にして、
bEnableExtEol のデフォルト指定を外したいっす。
bEnableExtEol のデフォルト指定を外すメリットは、#include "DLLSHAREDATA.h" する必要がなくなる、です。
また、デフォルト指定を外すことにより、共有メモリ（＝プロセス間をまたがって有効な超グローバル変数）に依存するコードの特定が容易になるっす。共有メモリに依存するコードは現実的にテスト不能なコードなので、テスト可能にできるようにして行ってる認識です。
もちろん、増やした引数のデフォルト指定をやめるってことは、呼出元を変更する必要が出てくるってことなので無理に対応しなくてよいと思っています。
There was a problem hiding this comment.
共有メモリ依存を明確にしていくのは確かに意義がありそうです。別PRで実施します。
|
ちなみに、残存した4件のCodeSmellsについては、「対応できない」で合意です。
超約： 行末のセミコロンが不要です。セミコロンがないとエラーになるようにassertマクロを修正するか、行末のセミコロンを削除してください。 セミコロンがないとエラーになるような、アフォな修正をassertマクロに加えることは容易です。 どちらの対応をしても「これ何？」でレビューされない気がするし、「なんでそんな修正しないといけないんだ！」とゴネる人が出そうな気がしました。 |
|
レビューありがとうございます。マージします。 |
標準のassertマクロでは |
|
ところでですが、assertマクロの独自定義は必要なものなんでしょうか。標準のassertを使っていない理由が気になってきました。 |
本来は不要、ここのプロジェクトでは必要、の認識です。 独自定義 assert が必要な理由は色々ありますが、端的には以下のような対応をしたことが原因です。 #1342 普通に考えれば「単体テストしづらくなるようなassert書くなよｗ」なんすけど、コードが先にある状態からテストを付けていってるので諦めるしかない気もします。 |
PR の目的
以下の CNativeW の static 関数に対するテストを追加します。
カテゴリ
PR のメリット
PR のデメリット
使用頻度の高い CCharWidthCache のメンバ関数を virtual にするため、パフォーマンスの低下が懸念されます。
ファイルを開いて拡大・縮小し、レイアウト生成を繰り返す手法で簡単にとったプロファイリングによれば、CCharWidthCache::CalcPxWidthByFont が CPU 時間の5～10%を占めますが、実行時間の98%程度が GDI の呼び出しであり、virtual 呼び出し自体はそれほど高価ではないようです。
PR の影響範囲
単体テストを可能にするため既存コードに引数を追加しますが、振る舞いを変更するものではありません。
関連 issue, PR
#1532