✅ Build sakura 1.0.2635 completed (commit 3b8584854e by @berryzplus)

✅ Build sakura 1.0.2636 completed (commit e2c66ea0ae by @berryzplus)

（このPRではなくそれ以前の）元実装に色々と気になる点があります。

元実装では書き込みモード時に DataProfile::IOProfileData メソッドを呼ぶ際に MakeStringBufferW0 マクロを使って StringBufferW 型のインスタンスを仲介する実装になっています。

どうしてそういう実装になっているのを推測するに DataProfile::IOProfileData メソッドから呼び出す文字列変換処理 DataProfile::value_to_profile メソッドの変換元の引数の型が const char* や const wchar_t* の場合のオーバーロードが無いからではないかと思います。

下記のメソッドが呼び出しされますが、StringBufferW::nDataCount は参照されないので元実装の
MakeStringBufferW0 マクロを使うやり方でも問題が起きないのかなと思います。

	void value_to_profile(const StringBufferW& value, wstring* profile)
	{
		*profile = value.pData;
	}

さてconst char* や const wchar_t* のオーバーロードが無いのなら追加すれば良いような気もしますが、もしかしたら何かの理由で意図的に追加していないのかもしれません。変換元の型が StringBufferW や StaticString の場合のオーバーロードは存在するので、変換元の型が文字列なのはイリーガルという設計意図は無いような気もしますが…よくわかりません。あと昔のC++には std::basic_string_view や std::array は存在しなかったかので色々な自前の型が定義して記述されてますね。文字列処理の記述に統一性が無いところは頑張ってなんとかしないといけないとこな気がしますが、やっぱ大変そうなので放置民です。

さてconst char* や const wchar_t* のオーバーロードが無いのなら追加すれば良いような気もしますが、もしかしたら何かの理由で意図的に追加していないのかもしれません。変換元の型が StringBufferW や StaticString の場合のオーバーロードは存在するので

const char* や const wchar_t* は単独ではサイズが不明だからかと思います。
StringBufferW や StaticString には書き込んでよいサイズ情報が含まれています。

まぁ、サイズが分かる型のみをサポートしてるはずなのにサイズチェックはしてないんで、溢れたら落ちる間抜け実装なんですよね 😄

このPRの背景には、溢れたら落ちるを解消したい、ってことも含まれています。
そのためには、StringBufferW をサイズ不明で構築させるマクロの存在が邪魔なんです 😃

IOProfileData系のメソッドが設定値に非const型を要求するのは、読み取りと書き込みの処理を別々に書いてどちらかを書き漏らすトラブルを防ぐためかと思っています。普通はやんないと思うのでそれって杞憂ですよね。

既存コードの突っ込みどころには、一括置換で対処できるもの以外できれば触れたくないです。
だからまぁ、IOProfileDataが1回だけ記述される構成にしてあるのは意図的なものです。

このPRが通ると、StringBufferWを「サイズが判明している書き込み可能なバッファを参照するクラス」に定義しなおせるようになります。

現状で5,6個ある文字列系クラスは、たぶん StringBuffer<T> の派生クラスとして定義しなおせる気がしています。最終的にそれをやるかどうかはPRの成否には関係ないと思ってますが。

さてconst char* や const wchar_t* のオーバーロードが無いのなら追加すれば良いような気もしますが、もしかしたら何かの理由で意図的に追加していないのかもしれません。変換元の型が StringBufferW や StaticString の場合のオーバーロードは存在するので

const char* や const wchar_t* は単独ではサイズが不明だからかと思います。
‘StringBufferWやStaticString` には書き込んでよいサイズ情報が含まれています。

元のコードを実装した人がNUL終端文字列をサポートしなかった理由はもはや想像の世界ですね…。

まぁ、サイズが分かる型のみをサポートしてるはずなのにサイズチェックはしてないんで、溢れたら落ちる間抜け実装なんですよね 😄

このPRの背景には、溢れたら落ちるを解消したい、ってことも含まれています。
そのためには、StringBufferW をサイズ不明で構築させるマクロの存在が邪魔なんです 😃

元の実装でそういうケースがあるという事でしょうか？ MakeStringBufferW0 マクロは CShareData_IO::ShareData_IO_Type_One メソッドで書き込みモードでしか使われていないのでその場合は問題無さそうに見えました。実際にそういうケースが有るかは不明だけど潜在的な不具合要因を回避するために存在自体を消したいという事でしょうか？

IOProfileData系のメソッドが設定値に非const型を要求するのは、読み取りと書き込みの処理を別々に書いてどちらかを書き漏らすトラブルを防ぐためかと思っています。普通はやんないと思うのでそれって杞憂ですよね。

読み書きのシリアライズ処理は工夫すれば読み書きの記述を結構共通化出来るのでサクラエディタの記述を見るともっとスマートに書けるだろう的に思ってしまいますが、現実的にはコード量が多いしケースバイケースなとこも多いから結局茨の道かもと思って変更諦めました。

既存コードの突っ込みどころには、一括置換で対処できるもの以外できれば触れたくないです。
だからまぁ、IOProfileDataが1回だけ記述される構成にしてあるのは意図的なものです。

うーん、でも分岐が多いと流れが追いにくいです…。まぁそんな長くないので無理難題ではないですが…。IOProfileDataの重複記述はクロージャを使えば解消出来ると思いますが、そうする事で可読性が上がるかは微妙なとこですね。

このPRが通ると、StringBufferWを「サイズが判明している書き込み可能なバッファを参照するクラス」に定義しなおせるようになります。

そもそもバッファ参照用の型なのに名前が StringBufferW なのが違和感が有るんですよね。その名前だとその型自体がバッファに思えてきます。

現状で5,6個ある文字列系クラスは、たぶん StringBuffer<T> の派生クラスとして定義しなおせる気がしています。最終的にそれをやるかどうかはPRの成否には関係ないと思ってますが。

自前型を用意しないでも std::wstring と std::wstring_view と std::array<wchar_t, N> の3つだけで置き換えは不可能ではないと思うんです。だけど既存コードの記述が様々なので書き換えするプログラミングのコストが膨大になると思います。

const char* や const wchar_t* は単独ではサイズが不明だからかと思います。
StringBufferW や StaticString には書き込んでよいサイズ情報が含まれています。

まぁ、サイズが分かる型のみをサポートしてるはずなのにサイズチェックはしてないんで、溢れたら落ちる間抜け実装なんですよね 😄

このPRの背景には、溢れたら落ちるを解消したい、ってことも含まれています。
そのためには、StringBufferW をサイズ不明で構築させるマクロの存在が邪魔なんです 😃

こちらですが実使用で溢れて落ちる挙動に入るケースが判明しているなら、再現確認をしたいので記載してもらえると助かります。というか仮に実使用で落ちるパターンがあってそれが修正されるならカテゴリがリファクタリングだけではないですね。

こちらですが実使用で溢れて落ちる挙動に入るケースが判明しているなら、再現確認をしたいので記載してもらえると助かります。というか仮に実使用で落ちるパターンがあってそれが修正されるならカテゴリがリファクタリングだけではないですね。

ここの使用例だと落ちません。

このコメント #1203 で指摘されてるとおり、 MakeStringBufferW0 の利用箇所は 読み取り側だけ だから、というのが理由です。

この点をクローズアップして対策すると CDataProfile::IOProfileData の利用をやめて、 CProfile::SetProfileDataImp を public にする感じの方向になります。

だからといって、このPRで提案している「 MakeStringBufferW0 の排除」にメリットがないかというと違う気がします。「MakeStringBufferW0 には害がある」というのはぼくの直観ですが、 実害 を立証できないので迷っている感じです。

あれ、というか approve もらってますね（ｷﾂﾞｲﾃﾅｶｯﾀ

ここの使用例だと落ちません。

このコメント #1203 で指摘されてるとおり、 MakeStringBufferW0 の利用箇所は 読み取り側だけ だから、というのが理由です。

自分が確認した限りでは MakeStringBufferW0 の利用箇所は CShareData_IO::ShareData_IO_Type_One において書き込みモード時のみでした。なので 読み取り側だけ だから というのは？？と思いました。まぁでもいちいち詳細確認して間違い起きないようにコメントするのはかったるいですね。

この点をクローズアップして対策すると CDataProfile::IOProfileData の利用をやめて、 CProfile::SetProfileDataImp を public にする感じの方向になります。

自分の考えはそれとは違います。まず CDataProfile::IOProfileData から呼び出している value_to_profile と profile_to_value をメソッドではなく関数にした方が良いと思います。これらの実装ではメンバー変数は参照していないですしおすし。あと公開関数にする事で別の型への対応を多重定義の関数を更に追加する事で行えます。

だからといって、このPRで提案している「 MakeStringBufferW0 の排除」にメリットがないかというと違う気がします。「MakeStringBufferW0 には害がある」というのはぼくの直観ですが、 実害 を立証できないので迷っている感じです。

下記のマクロ実装は時代遅れなので排除する事自体は良いと思います。

//文字列バッファ型インスタンスの生成マクロ
#define MakeStringBufferW(S) StringBufferW(S,_countof(S))
#define MakeStringBufferW0(S) StringBufferW(S,0)

オーバーロードするコンストラクタでテンプレート引数から固定長配列の要素数は取得出来るので、MakeStringBufferW マクロも使わないで良いんじゃないかと思います。

ただあえて動作しているものを無暗に触るなと〆られる可能性も…。

自分が確認した限りでは MakeStringBufferW0 の利用箇所は CShareData_IO::ShareData_IO_Type_One において書き込みモード時のみでした。なので 読み取り側だけ だから というのは？？と思いました。

設定ファイルに書き込むモードは、設定値から値を読み取るモードです。
逆に、設定ファイルから読み込むモードは、設定値に値を書き込むモードです。

MakeStringBufferW0 の問題は、設定値に値を書き込みたいときに書き込んでよいサイズが不明である点にあります。実際の利用が、設定値から値を読み取る場合のみに使われているので問題ないって話でした。

分かりづらいコメントしててすんません。

まず CDataProfile::IOProfileData から呼び出している value_to_profile と profile_to_value をメソッドではなく関数にした方が良いと思います。これらの実装ではメンバー変数は参照していないですしおすし。あと公開関数にする事で別の型への対応を多重定義の関数を更に追加する事で行えます。

その変更自体は次のPRで予定しています。設定値と std::wstring との相互変換を public で static な公開関数として他のクラスからも呼べるようにしたいと思っています。

オーバーロードするコンストラクタでテンプレート引数から固定長配列の要素数は取得出来るので、MakeStringBufferW マクロも使わないで良いんじゃないかと思います。

これは正直やりたいんですが、いっぱい変更しているように見えるので次のPRには含めないかもしれません。

ん？完全なフリー関数と public static なメンバ関数って違う・・・？ｗ

レビューありがとうございました。
次ステップに進みたいと思います。
何か問題が見つかったらいつも通り別PRで対処していきたいと思います。

ん？完全なフリー関数と public static なメンバ関数って違う・・・？ｗ

C言語で static 関数というとファイルスコープに限定した関数ですね。
public static なメンバ関数とはスコープが違うし標準C++には partial class も無いので拡張しにくいです。
C++のTMPでMixin頑張るのは闇要素高いのでフリー関数のオーバーロードでリンク時に解決が単純で良い気がします。

ではフリー関数として公開する方法で精査していきます。

名前重複があるとめんどいんで namespace を使う感じになると思います。

検討している候補は basis、sakura、skr...

設定ファイル読込イメージ

std::wstring profile = L"255";
int value = 0;
bool ret = basis::TryParse( profile, value ); //元profile_to_value関数

設定ファイル書込イメージ

int value = 255;
std::wstring profile;
profile = basis::ConvertToString( value ); //元value_to_profile関数

PRまでには2～3日かかると思います。