メッセージボックス関数の単体テスト向け独自拡張の不具合を修正したい

[berryzplus]

PR の目的

タイトル通りです。

カテゴリ

• その他の問題
  テスト専用コードの不具合を修正します。

PR の背景

Wrap_MessageBoxに不具合があることが分かりました。

通常、メッセージボックスの表示を含むコードは自動テストできません。
サクラエディタにはメッセージボックスを表示するコードが多いので、
「メッセージボックスを表示する＝テストできません」だと困ってしまいます。
なので #1362 で Wrap_MessageBox を改造し、表示しようとしたメッセージをコンソールに出力するようにしました。

ところがテストプログラムでコンソールに出力したメッセージの取得を試みたところ、うまくいきませんでした。(#1812)

テストで使えるように入れた機能がテストプログラムから簡単に扱えないのは、たとえ仕様的には正しくても不具合だと思うので、不具合として修正したいと思います。

PR のメリット

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

仕様・動作説明

アプリの本体コード内に埋め込んだテスト専用コードを修正します。
アプリの仕様・挙動に影響を与える修正ではありません。

1. 変更点 Wrap_MessageBox
   旧）コンソールへの出力に WriteConsoleW を利用。gtestでキャプチャしようとすると失敗した。
   新）コンソールへの出力に std::clog を利用。gtestでキャプチャできるようになった。
   コンソールへの出力に std::wcerr を利用しようとしたところ、出力された文字列を gtest で取得できなかった。
2. 変更点 Wrap_MessageBox
   旧）コンソールへの出力時に wchar_t* の文字列をそのまま出力していた。
   新）コンソールへの出力時に wchar_t* の文字列を UTF-8 に変換して出力するように変えた。
   ACP(=Windows-31J、シフトJIS)に変換する仕様とすると「変換できない問題」が発生するので UTF-8 を採用した。
3. 変更点 VMessageBoxF
   旧）固定長16000文字の内部バッファを使ってフォーマットしていた。 VMessageBoxFのバッファ長 #1416 の通り、バッファサイズが明らかに大きすぎで、しかも、フォーマット結果が15999文字を超えた場合にはメッセージが切り捨てられてしまう制約があった。
   新）フォーマット関数を strprintf に変更し、動的にバッファ確保するように変えた。

PR の影響範囲

テスト専用コードの不具合修正なので、アプリの挙動には影響しません。
不具合が解消することにより「処理結果としてメッセージ表示するコード」の単体テストを書けるようになります。
追加したwcsとu8sの相互変換関数を、アプリ内で転用することができるようになります。

テスト内容

このPRは改修部分の単体テストコードを含んでいます。

テスト1

手順

関連 issue, PR

• resolves CPPA::stdErrorのテストを追加するにあたっての課題共有 #1812
• resolves VMessageBoxFのバッファ長 #1416
• fixes 単体テストで起きたassert失敗を捕捉できるように、コンソールモードではメッセージボックスを表示しないようにしたい #1362

参考資料

• GoogleTest で標準出力、標準エラー出力をテストする

[AppVeyorBot]

✅ Build sakura 1.0.4073 completed (commit 621a8d1932 by @berryzplus)

[AppVeyorBot]

✅ Build sakura 1.0.4074 completed (commit cf80443eef by @berryzplus)

[ghost]

ところで、C++11以降ではchar型とwchar_t型のほかに、char16_t型とchar32_t型が追加されています。
char16_tはUTF-16で、char32_tはUTF-32でエンコーディングされた文字を想定したデータ型です。
さらにC++20では、UTF-8エンコーディングを想定したchar8_t型も追加されました。
https://cpprefjp.github.io/lang/cpp11/char16_32.html
https://cpprefjp.github.io/lang/cpp20/char8_t.html

これらのデータ型に格納された文字とマルチバイト文字との相互変換に使える関数は<cuchar>ヘッダに次のように定義されています。

namespace std {
  size_t mbrtoc8(char8_t* pc8, const char* s, size_t n, mbstate_t* ps); // since C++20
  size_t c8rtomb(char* s, char8_t c8, mbstate_t* ps); // since C++20
  size_t mbrtoc16(char16_t* pc16, const char* s, size_t n, mbstate_t* ps); // since C++11
  size_t c16rtomb(char* s, char16_t c16, mbstate_t* ps); // since C++11
  size_t mbrtoc32(char32_t* pc32, const char* s, size_t n, mbstate_t* ps); // since C++11
  size_t c32rtomb(char* s, char32_t c32, mbstate_t* ps); // since C++11
}

（出典： https://en.cppreference.com/w/cpp/header/cuchar ）

このPRでは（C++20でも用意されていない）ワイド文字とUTF-8間の変換関数を追加するわけですが、関数名をc8stowcsのように変更して、<cuchar>の定義に似せるのはいかがでしょうか？

[berryzplus]

https://cpprefjp.github.io/lang/cpp20/char8_t.html
ここを見る限り、 std::basic_string<char8_t> の別名は std::u8string になるようなので、u8s はそのままでいい気がしています。引数型を std::u8string や std::u8string_view に変えることで分かりやすさを改善できそうなので、修正試みてみます。

[AppVeyorBot]

❌ Build sakura 1.0.4075 failed (commit bb6d7cd7fd by @berryzplus)

[berryzplus]

std::u8string に変える修正は少しハードル高いので、一旦放置します。

• https://docs.microsoft.com/en-us/cpp/build/reference/zc-char8-t?view=msvc-170
• https://docs.microsoft.com/ja-jp/cpp/build/reference/std-specify-language-standard-version?view=msvc-170
• 使用する言語規格に C++17 を指定 #986

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[berryzplus]

Code Factorがエラーを吐いてるのはいつも通りなのでスルーで。

おそらく「ファイル内の累積複雑度が基準値を超えている」という警告。
要らない関数を削ってスリムアップすることには意味がありそうですが、
「このPRの問題か？」というとそうではないので放置してしまいます。

[AppVeyorBot]

✅ Build sakura 1.0.4076 completed (commit bb6d7cd7fd by @berryzplus)

[ghost]

https://cpprefjp.github.io/lang/cpp20/char8_t.html ここを見る限り、 std::basic_string<char8_t> の別名は std::u8string になるようなので、u8s はそのままでいい気がしています。

<cuchar>ヘッダの既存関数と命名規則を揃えれば互いに存在を連想できると思ったので言ってみました。
「u8~の方がわかりやすい」ならばそれでも良いです。

[ghost]

引数型を std::u8string や std::u8string_view に変えることで分かりやすさを改善できそうなので、修正試みてみます。

std::u8string に変える修正は少しハードル高いので、一旦放置します。

* https://docs.microsoft.com/en-us/cpp/build/reference/zc-char8-t?view=msvc-170

* https://docs.microsoft.com/ja-jp/cpp/build/reference/std-specify-language-standard-version?view=msvc-170

* [使用する言語規格に C++17 を指定 #986](https://github.com/sakura-editor/sakura/pull/986)

現時点ではC++20は採用できません（＝char8_tは使えません）ので、放置するのが正解です。

C++20の<format>・<ranges>・<chrono>以外はVS2019 16.10以降が必要です。
なお、<format>・<ranges>・<chrono>はVS2022 17.2でようやく利用可能になるようです。
https://github.com/microsoft/STL/wiki/Changelog#expected-in-vs-2022-172-preview-2
VS2019にもバックポートされる予定はありますが、これに関してはいつになるかわかりません。

GCCは10.1以降なら使えるらしいですが、こっちのことはよく知りません。
CMakeは3.20.4からです。

あとCAppNodeGroupHandle::operator==を廃止しないとビルドできません（すでに試した）。
C++20では三方比較演算子の導入に伴って同値比較演算子の仕様が変わり、operator==からoperator!=が導出されるようになりました。
このためか、独自の比較演算子の定義が曖昧になっています。
（※三方比較演算子と新仕様の同値比較演算子については https://cpprefjp.github.io/lang/cpp20/consistent_comparison.html を参照してください。）

[ghost]

テストも通っていますし、LGTMです。

[ghost]

<format>・<ranges>・<chrono>はVS2022 17.2でようやく利用可能になるようです。

僕の方ではVS2017の廃止対応が済み、17.2がリリースされるまでC++20の導入を提案しないつもりですが、他の方からの導入要望は歓迎します。

[berryzplus]

レビューありがとうございます。マージしちゃいます。
何か不都合あれば別PRで対処していきたいと思います。

[berryzplus]

・・はVS2022 17.2でようやく利用可能になるようです。

僕の方ではVS2017の廃止対応が済み、17.2がリリースされるまでC++20の導入を提案しないつもりですが、他の方からの導入要望は歓迎します。

「なんちゃって C++20 」で構わなければ、ビルドオプションで /std:latest とか指定すれば vs2017 でも一部の C++20 を使える認識です。「なんちゃって」でどこまで対応できるかは謎ですが std::u8string 程度ならイケるのではないか？という予想です。。。

あと、記憶が確かなら <chrono> というヘッダは既に使っていたような気がします。
パフォーマンス計測に使うヘッダで、 C++20 で導入された機能とは違うかもしれませんが。

[ghost]

「なんちゃって C++20 」で構わなければ、ビルドオプションで /std:latest とか指定すれば vs2017 でも一部の C++20 を使える認識です。「なんちゃって」でどこまで対応できるかは謎ですが std::u8string 程度ならイケるのではないか？という予想です。。。

https://docs.microsoft.com/ja-jp/cpp/overview/visual-cpp-language-conformance?view=msvc-170
/std:latestも含めればVS16.2ですね。ただし、std::basic_ostream::operator<<についてはVS16.6です。

あと、記憶が確かなら <chrono> というヘッダは既に使っていたような気がします。 パフォーマンス計測に使うヘッダで、 C++20 で導入された機能とは違うかもしれませんが。

<chrono>はC++11から導入され、C++20で拡張されました。
https://en.cppreference.com/w/cpp/chrono

[berryzplus]

u8string は vs2017 を「足切り」しない限り導入できない、と理解しました。

ホント言うと、wcs も u8s も mbs も「迂闊に使うとエディタ仕様を損なう危険がある」と思っているので、
仮に std::u8string が使えるようになっても積極的に使うべきではないと考えています。

じゃ、なんで入れとんねん！　ってことになりますが、
「テストの仕組みとして必要」だから入れた　になるわけで、
必要じゃなかったら入れたりしないわけで・・・。