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

SonarCloud解析で検出されたゼロ除算コードに対策する #1511

Merged

Conversation

berryzplus
Copy link
Contributor

PR の目的

SonarCloud解析で検出されたゼロ除算コードに対策します。
結果的に #1510 すべてのウィンドウが最小化された状態で「並べて表示」コマンドを実行するとクラッシュする の不具合が直ります。

カテゴリ

  • 不具合修正

PR の背景

SonarCloud解析でBugだと言われている件に対処したい #1504 と関連します。

「SonarCloud解析を便利に使えるように」対応の下準備として、簡単に直せる指摘について対処策を考えていました。

#1510 で実害があると報告されたので対処を入れる感じです。

PR のメリット

不具合を修正することができます。

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

直し方が汚いと思います。(100%主観)
直し方が汚いと感じるのは、もっと言い直し方がある(たぶん)という直感の現れだと思います。
警告を消すためだけの対応を行うことで、本来採るべき最適解を探る機会が失われることがデメリットです。

仕様・動作説明

「ゼロで割る」は、最も有名なCPUが実行できない命令です。
これをやったら例外キャッチする機構がない限り、プログラムは落ちます。

上下に並べて表示、左右に並べて表示の実装は、デスクトップ領域を「表示されているウインドウの数」で割って1ウインドウあたりの高さ(幅)を算出し、オフセットで並べる感じになっています。表示されているウインドウが0個だった場合、ウインドウ領域の高さ(幅)をゼロで割ることになるので、ウインドウ数が0の場合を除外しないと落ちてしまいます。

修正コードでは、単にreturnするのではなく、
関数内で独自に確保した配列メモリの解放も行うようにしています。

PR の影響範囲

  • 表示しているウインドウがない状態で「並べて表示」を実行したときの挙動に影響します。

テスト内容

以下のファイルを test1.js などのテキトーなファイル名で保存し、キーマクロの読み込みをしてからキーマクロの実行をします。実行するとすべてのサクラエディタウインドウが最小化されます。修正前はその後プログラムがクラッシュしていましたが、修正後はクラッシュしなくなります。

Editor.MinimizeAll();
Editor.TileWinV();  // 上下に並べて表示
Editor.MinimizeAll();
Editor.TileWinH();  // 左右に並べて表示```

関連 issue, PR

fix #1510
#1504

参考資料

@berryzplus berryzplus added 🐛bug🦋 ■バグ修正(Something isn't working) SonarQube labels Jan 20, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jan 20, 2021

@AppVeyorBot
Copy link

Copy link
Member

@kengoide kengoide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRにしていただきありがとうございます。問題が修正されることを確認しました。

コードについても最小限の修正としてならば言うことはないと思われます。SonarCloud の警告が増えるのが残念ですけれど、最小限なので。

直し方が汚いと思います。(100%主観)
直し方が汚いと感じるのは、もっと言い直し方がある(たぶん)という直感の現れだと思います。
警告を消すためだけの対応を行うことで、本来採るべき最適解を探る機会が失われることがデメリットです。

#1510 の内容も含めて、そういう言及があったことを覚えておきます。

@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit d1bd398 into sakura-editor:master Jan 23, 2021
@berryzplus berryzplus deleted the feature/fix_devide_by_zero branch January 23, 2021 05:14
@berryzplus
Copy link
Contributor Author

berryzplus commented Jan 30, 2021

この変更に起因するデグレが2chで報告されています。

268 名無しさん@お腹いっぱい。2021/01/23(土) 18:58:22.09ID:UBVVx7900
1.0.3360以降でsakuraCalc.vbs Ver1.0が実行出来なくなった

明らかに関係ない変更なので、真偽は不明。

@kengoide
Copy link
Member

1.0.3360 なら #1512 の可能性があると思います。

@berryzplus
Copy link
Contributor Author

1.0.3360 なら #1512 の可能性があると思います。

いやビルド番号が 1.0.3360 なら、このPRのマージコミットです。
d1bd398

もっとも、2chに報告されたビルド番号が間違ってる可能性は高いです。
発生事象が一切不明なので微妙ですが #1512 は設定を入出力するマクロの挙動に影響する余地があります。

2chで聞いてみようか迷っています。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working) SonarQube
Projects
None yet
3 participants