StaticString::Length()の戻り値をintにキャストする

[berryzplus]

PR の目的

x64対応のために、x64ビルドで発生する警告に対処します。

カテゴリ

• リファクタリング

PR の背景

#1541 で x64 対応ブームが始まったので対応してみます。

PR のメリット

x64 ビルドの警告が 2個減ります。

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

とくにありません。

仕様・動作説明

変更対象は、独自定義の拡張配列に格納された文字数を返す関数です。

独自関数auto_strlen()はsize_tの値を返しますが、関数の戻り型定義がintであるため、
暗黙の縮小変換となってコンパイラが警告を吐いています。

Platform	int型のサイズ	size_t型のサイズ
Win32	32bit	32bit
x64	32bit	64bit

StaticStringはスタックまたは共有メモリ上に固定サイズの配列を確保するための型です。

文字列長がINT_MAXな場合、配列サイズは4GBになります。
「スタックに4GB確保」はありえないので、問題になる可能性があるのは共有メモリのみになります。
x64化したからといって、共有メモリにそんなバカでかいデータを載せたいかというと、それもありえないと思います。

発生しているのは暗黙の縮小変換に対する警告のみです。

PRでは、より安全性の高い auto_strnlen を使うように修正しています。

PR の影響範囲

実害はありません。

テスト内容

すでに作成している単体テストがカバーする範囲の修正であるため、追加のテストは不要と考えています。

関連 issue, PR

#1563
#1541

参考資料

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[AppVeyorBot]

✅ Build sakura 1.0.3503 completed (commit 4de08c1736 by @berryzplus)

[beru]

本来は負の値を取らない長さやサイズ用の型なので色々と size_t にするべきだと思います。

動的確保はしない用途向けの型なので実際に INT_MAX より大きい値が使われる事は無いと思いますが、宣言上の型は明確に用途に適したものにした方が良いかなと…。

分かりやすく例えていうとJRAの馬運車の見た目が右翼の街宣車になっていても機能的には問題無いかもしれないけれど適さないという外観上の話です。

[berryzplus]

本来は負の値を取らない長さやサイズ用の型なので色々と size_t にするべきだと思います。

最終的には StaticString::Length() の戻り値自体を size_t にすべきだと思います。

それをやると修正範囲が広がってしまうので、このPRは内部的な縮小変換にキャストを付けることにより、警告を潰そうとしています。「潰しても問題ない」と判断するのに十分な客観的な理由を提示できていると考えていて、「警告を潰すためにやみくもに付けるキャスト」とは異質だと思っています。

[sanomari]

x64考慮が不十分でコンパイラ警告が出てしまう件は解決できてそうに見えました。

「サイズを返すメソッドの戻り型にintを採用しているのは誤りかも？」には同感ですが、x64対応とは関係ない話題だと思います。

[beru]

本来は size_t 型を使うべきところを int にキャストする対応で済ませてはいけないという原理主義に走ると修正に手間が掛かってしまい警告除去がなかなか進まないのは確かですね。なのでその考えに陥らないように気を付けたいと思います。

[beru]

その対応で問題ないと思います。

[berryzplus]

レビューありがとうございます。マージしちゃいます。

[kengoide]

本来は負の値を取らない長さやサイズ用の型なので色々と size_t にするべきだと思います。

これについては近年対立する意見が浸透してきています。signed と unsigned との変換はバグの温床であるため最小限にするべきものですが、最終的な結果が常に正の値になる計算であっても中間結果が負になることは考えられるため、可能な限り signed に統一したほうがよい、というものです。std::ssize が C++20 に入ったのはこの手の主張を反映しています。Google C++ Style Guide は size_t を historical accident とまで呼んでいたりします。

私個人の感覚としては、int で事足りるなら int のままで良いと思います。unsigned という十分に注意しないとバグが出る型は使わないに越したことないですし、値域が足りないなら1ビット増やしても焼け石に水というやつだと思います。

[kengoide]

値域が足りないなら1ビット増やしても焼け石に水というやつだと思います。

この発言は変でしたね。値域が足りないならビット幅を明示したほうがいい、に訂正します。

[beru]

本来は負の値を取らない長さやサイズ用の型なので色々と size_t にするべきだと思います。

これについては近年対立する意見が浸透してきています。signed と unsigned との変換はバグの温床であるため最小限にするべきものですが、最終的な結果が常に正の値になる計算であっても中間結果が負になることは考えられるため、可能な限り signed に統一したほうがよい、というものです。std::ssize が C++20 に入ったのはこの手の主張を反映しています。Gooogle C++ Style Guide は size_t を historical accident とまで呼んでいたりします。

私個人の感覚としては、int で事足りるなら int のままで良いと思います。unsigned という十分に注意しないとバグが出る型は使わないに越したことないですし、値域が足りないなら1ビット増やしても焼け石に水というやつだと思います。

おお、勉強になります。説明ありがとうございます。

unsigned型は気を付けないと不具合を生むよというのはその通りですね。やねうらおさんも昔ブログに書いていた気がします。unsigned型の変数の四則演算でも減算の記述が有ると被減数より減数の方が大きい場合に演算結果が本来はsigned型で表現するべき値になるので問題になりますね。プログラマが気を使ってなるべく不具合が無いようにするべきですが人間なので見逃す事はよくあると思います。

clang だと -Wsign-conversion で暗黙の符号変換を警告する事が出来るようで人によっては暗黙の符号変換は不具合の元になるという事でつぶしているようです。UndefinedBehaviorSanitizer とかでランタイムに怪しいところを検出させたり、手間ですが実行時に範囲チェックをするように明示的に書くという方法も取れますね。

int で事足りるならというのはそうなんですが、元々 size_t を返す箇所に int へのキャストを入れて int 型の変数で受けるコードばかりにしたくはないなという思いはあります。とはいえ size_t 型の変数を使うと今度はその変数を渡す箇所の型が int の場合にキャストを入れる必要が出てきますが…。

x64ビルドでの警告除去はケースバイケースでの対応になると思いますが、警告を無くすために size_t に型を変更すると影響範囲が連鎖して収拾が付かなくなりそうな箇所もあるのでそういうところは int へのキャストで済ませたほうが良さそうな気がします。とはいえ32bitを超える大きい値を扱えるようにする事でメリットが出てくる箇所はx64ビルドでの利点を生かす対応にしたいですね。