Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CheckSjisChar 関数の第2引数の型を int から size_t に変更 #1556

Merged
merged 1 commit into from
Feb 28, 2021

Conversation

beru
Copy link
Contributor

@beru beru commented Feb 28, 2021

PR の目的

x64 ビルドで警告が出る数を減らす為の変更です。

カテゴリ

  • リファクタリング

PR の背景

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

PR のメリット

sakura プロジェクトの x64 Debug ビルド時の警告の数が 607 から 605 になり、2個減ります。
x64 Release ビルド時の警告の数が 572 から 570 になり、2個減ります。
Win32 Debug と Win32 Release ビルド時の警告の数は 0 のままです。

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

意識的に色々な型を使ってコードを書きたくない人にとってはコードの可読性は落ちると思います。

仕様・動作説明

関数の呼び出し元では pr_end-pr という記述でバッファ長を求めています。この演算の結果の型はいわゆる ptrdiff_t ですが、Win32 ビルドでは int なので元の実装でも警告は出ませんが、x64 ビルドでは int64_t 相当なので引数の型が int のままだとビット幅を狭めるデータ変換 (narrowing conversion) になり警告が出ます。

なお呼び出し元における pr_end の値の作り方から考えて演算結果が負の値になる事はおそらく無いと考えています。

これに関しては本当に負の値が生じない組み方になっているかをコンパイル時にきちんと検証はしていません(C++言語仕様的にどこまで出来るかは不明、可能だとしても形式的検証を取れる記述が容易とも思えない)。

boost::numeric_cast のように実行時に確認するというのも選択肢に上がると思いますが、実行時エラーが絶対に許容されない用途向けのプログラムではないと思うので省きます。

PR の影響範囲

CheckSjisChar 関数とそれを呼び出している CESI::GetEncodingInfo_sjisCShiftJis::SjisToUni です。

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

CShiftJis::SjisToUni の呼び出し元をたどっていく文字コード変換処理になります。

テスト内容

テスト1

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

テスト2

  • サクラエディタを起動する
  • README.md ファイルを開く
  • SJISで保存した README.md ファイルを開いて正常に読み込めることを確認する
  • メニューの変換 > 文字コード変換 の SJIS から違うコードへの変換を行った後に、SJISへのコード変換を行い元に戻る事を色々確認

関連 issue, PR

#430 #1541 #1555

x64 ビルドで警告が出る数を減らす為の変更
関数の呼び出し元では pr_end-pr という記述でバッファ長を求めており、 pr_end の値の作り方から考えて演算結果が負の値になる事は無いと思われる
@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Feb 28, 2021
@AppVeyorBot
Copy link

Build sakura 1.0.3486 completed (commit 93486d1ceb by @beru)

@beru
Copy link
Contributor Author

beru commented Feb 28, 2021

SonarCloud / Scan でテストが失敗していますが原因は不明です。ローカルで実行しても再現しません。

[ RUN      ] ParameterizedTestWinMain/WinMainTest.runWithNoWin/1
[info] Module: D:\a\sakura\sakura\tests\build\x64\Debug\unittests\tests1.exe is selected because it matches selected pattern: D:\a\sakura\sakura\tests\build\x64\Debug\unittests\tests1.exe
D:\a\sakura\sakura\tests\unittests\code-main.cpp(65): launching process ["D:\a\sakura\sakura\tests\build\x64\Debug\unittests\tests1.exe" -PROF="profile1" -NOWIN]
unknown file: error: C++ exception with description "waitEvent is timeout." thrown in the test body.
[  FAILED  ] ParameterizedTestWinMain/WinMainTest.runWithNoWin/1, where GetParam() = L"profile1" (10007 ms)

@berryzplus
Copy link
Contributor

SonarCloud / Scan でテストが失敗していますが原因は不明です。ローカルで実行しても再現しません。

すみません、そのエラーは無視してください。
issue #1535 で言ってる「たまに失敗する」の事象です。

とりあえず "waitEvent is timeout." のタイムアウト値を2倍にすれば「発生しづらくなる」ことが分かっていますが、まだ原因不明です。JobのRe-Runだけかけておきました。

@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@beru
Copy link
Contributor Author

beru commented Feb 28, 2021

SonarCloud / Scan でテストが失敗していますが原因は不明です。ローカルで実行しても再現しません。

すみません、そのエラーは無視してください。
issue #1535 で言ってる「たまに失敗する」の事象です。

とりあえず "waitEvent is timeout." のタイムアウト値を2倍にすれば「発生しづらくなる」ことが分かっていますが、まだ原因不明です。JobのRe-Runだけかけておきました。

コメントありがとうございます。クラウドサーバーが混雑していたのかもしれないですね。

@beru beru merged commit ef9ce32 into sakura-editor:master Feb 28, 2021
@beru beru deleted the x64_fix_warning_CheckSjisChar branch February 28, 2021 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants