DetectJisEscseq 関数の第2引数の型を int から size_t に変更

[beru]

第２引数の型の種類を変更したのと、関数呼び出し時に引数の型が異なる為に出る警告を無くすためにキャストを追加しました。

PR の目的

x64 ビルドで警告が大量に出るのでそれを減らす事が目的です。

このPRでは１つの警告しか減りませんが、千里の道も一歩から、という事で少しずつでも減らしていければと思います。

今回変更した関数の呼び出し元の _CheckJisAnyPart 関数の第2引数が「チェック対象となるバッファの長さ」ですがこれが int です。追っていくと CESI::SetInformation 以下の呼び出しがバッファ長の引数の型が int なので size_t に変えるべきです。

カテゴリ

• リファクタリング

PR の背景

x64 のビルドで警告が大量に出る事が #430 で話されています。

PR のメリット

ビルド時の警告が１つ減ります。

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

あまり色々な型を区別して使いたくない人にとってはコードの可読性は落ちると思います。

ptrdiff_t から size_t への cast は functional notation で行っていますが、人によって好き嫌いがありそうです。

仕様・動作説明

DetectJisEscseq 関数は _CheckJisAnyPart 関数から呼び出しています。
第2引数に渡す値は pr_end - pr というポインタの差分の演算で作られます。
ポインタの差分値の型は ptrdiff_t という typedef された型で扱うべきですが、定義は以下のようになっています。

// Definitions of common types
#ifdef _WIN64
    typedef unsigned __int64 size_t;
    typedef __int64          ptrdiff_t;
    typedef __int64          intptr_t;
#else
    typedef unsigned int     size_t;
    typedef int              ptrdiff_t;
    typedef int              intptr_t;
#endif

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\include\vcruntime.h からコピペしました。

for ループの前で pr と pr_end の値は下記のようにして設定されています。

	pr = pS;
	pr_end = pS + nLen;

そして nLen 変数の型は int なので pr_end - pr の演算結果の値は必ず int に収まるので暗黙的に int にキャストしても実質的に問題は無いはずなんですが、コンパイラはそういう背景はお構いなしに警告を出します。

前置きが長くなりましたがこのPRでは pr_end - pr の結果を size_t にキャストしています。DetectJisEscseq 関数の第2引数の型を変更したのに合わせていますが、どうして int から size_t に変えたかというと一般的にバッファ長等の負の値を取りえない長さの値の型は size_t にするからです。処理内容的には文字列の先頭にあるかもしれないエスケープ文字列を検出するだけなので int 型で十分サイズは収まって問題はありませんが、警告を取るためだけに変更を行いました。

PR の影響範囲

DetectJisEscseq 関数とそれを呼び出している _CheckJisAnyPart 関数です。

呼び出し元をたどっていくと日本語コードセット判定を行う CESI::CheckKanjiCode になり、文字コード種別の判定で使われます。

テスト内容

テスト1

• サクラエディタを起動する
• README.md ファイルを開く
• README.md ファイルを文字コードセット JIS でデスクトップに保存（ファイル > 名前を付けて保存）
• サクラエディタを終了する
• サクラエディタを起動する
• JISで保存した README.md ファイルを開いて正常に読み込めることを確認する

関連 issue, PR

#430 #1541

[AppVeyorBot]

✅ Build sakura 1.0.3483 completed (commit bae6eb9b86 by @beru)

[berryzplus]

PR趣旨に賛同します。
不要なキャストを入れてる気がするので、そこを気にして一旦コメントにします。

なんかの間違いかと思いますが、一応以下突っ込んでおきます。

for ループの前で pr と pr_end の値は下記のようにして設定されています。

	pr = pS;
	pr_end = pS + nLen;

そして nLen 変数の型は int なので pr_end - pr の演算結果の値は必ず int に収まるので暗黙的に int にキャストしても実質的に問題は無いはずなんですが、コンパイラはそういう背景はお構いなしに警告を出します。

ここがダウトです。

nLen 変数の型は int なので pr_end - pr の演算結果の値は必ず int に収まる

prもpr_endもポインタなので、数値じゃないです。
ポインタの加減算の演算結果は ptrdiff_t になります。
WindowsはLLP64 なので x64 だと ptrdiff_t が int に収まらなくなります。

なお、該当コードは単体テストのカバー範囲に含まれるため、追加のテストは不要と思います。

[berryzplus]

第２引数の型の種類を変更したのと、関数呼び出し時に引数の型が異なる為に出る警告を無くすためにキャストを追加しました。

ん？これは「キャストを付けないと警告が増える」と言ってますかね。

[berryzplus]

実際にビルドして確認しました。

バージョン	警告数
変更前	608
このPR	607
修正提案ver	607

以上により、charset/codechecker.cpp の 371 行目のキャスト追加は不要と考えられます。

[k-takata]

nLen 変数の型は int なので pr_end - pr の演算結果の値は必ず int に収まる

prもpr_endもポインタなので、数値じゃないです。
ポインタの加減算の演算結果は ptrdiff_t になります。
WindowsはLLP64 なので x64 だと ptrdiff_t が int に収まらなくなります。

型の話ではなくて取り得る値の範囲の話だと思いますが。
pr_end - pr の取り得る最大値は nLen ですので演算結果は int の範囲内に収まりますね。

[berryzplus]

nLen 変数の型は int なので pr_end - pr の演算結果の値は必ず int に収まる

prもpr_endもポインタなので、数値じゃないです。
ポインタの加減算の演算結果は ptrdiff_t になります。
WindowsはLLP64 なので x64 だと ptrdiff_t が int に収まらなくなります。

型の話ではなくて取り得る値の範囲の話だと思いますが。
pr_end - pr の取り得る最大値は nLen ですので演算結果は int の範囲内に収まりますね。

すみません、なんか違う話をされていますかね。
コンパイラが検出できるのは、型のミスマッチだけだと思っています。
値域の話は、他に色々考えることがある気がしますが、とりあえず気にしていません。

sakura/sakura_core/charset/codechecker.cpp

Line 356 in b2178a2

const char *pr, *pr_end;

宣言型がポインタである以上、引き算の結果型は ptrdiff_t になるはずっす。
それを int 型に渡す場合、x64では縮小変換になるので明示的なキャストが必要です。
それを size_t 型に渡す場合、x64でも縮小変換にはならないので明示的なキャストは不要です。

[beru]

レビューありがとうございます。

ptrdiff_t から size_t への暗黙的な変換で警告が出ると思ったのは自分の勘違い（というか理解不足）で、指摘して頂いた通りキャストを入れる必要はありませんでした。

https://docs.microsoft.com/en-us/cpp/cpp/type-conversions-and-type-safety-modern-cpp?view=msvc-160#signed---unsigned-conversions

The compiler does not warn about implicit conversions between signed and unsigned integral types.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[AppVeyorBot]

✅ Build sakura 1.0.3484 completed (commit b9f0381ded by @beru)