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

CCommandLine::GetFileNumの戻り値をintにキャストする #1563

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

berryzplus
Copy link
Contributor

PR の目的

カテゴリ

  • リファクタリング

PR の背景

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

PR のメリット

x64 ビルドの警告が 13個ほど減ります。

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

とくにありません。

仕様・動作説明

変更対象は、コマンドラインから指定した2個目以降のファイル名を格納したvectorのサイズを返す関数です。

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

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

PR の影響範囲

実害はありません。

x64ビルドのメリットとして「コマンドラインに20億個以上のファイル名を指定できる」を売りにできる可能性を潰す修正になります。

テスト内容

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

関連 issue, PR

#1541

参考資料

@@ -84,7 +84,7 @@ class CCommandLine : public TSingleton<CCommandLine> {
tagSIZE GetWindowSize() const noexcept { return { m_fi.m_nWindowSizeX, m_fi.m_nWindowSizeY }; }
tagPOINT GetWindowOrigin() const noexcept { return { m_fi.m_nWindowOriginX, m_fi.m_nWindowOriginY }; }
LPCWSTR GetOpenFile() const noexcept { return m_fi.m_szPath; }
int GetFileNum(void) const noexcept { return m_vFiles.size(); }
int GetFileNum(void) const noexcept { return int(m_vFiles.size()); }
Copy link
Member

Choose a reason for hiding this comment

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

static_cast を使わず、あえて function style でキャストする意図が汲めませんでした。

Copy link
Member

Choose a reason for hiding this comment

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

Sonar的には Major Code Smell だそうです。
https://rules.sonarsource.com/cpp/RSPEC-871

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sonar的には Major Code Smell だそうです。
https://rules.sonarsource.com/cpp/RSPEC-871

初回の修正で検出されなかったのが謎なんですよね。

これまでやった対応からすると [[nodiscard]]を付けないと怒られそうな気配があるんですけど、それも現段階では検出されていないような。

マージ後のscanで修正部分以外もチェックしたら新たに警告が出るのかもしれないので、そのあたりは出たとこ勝負でいこうと思っています。

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.

妥当だと思います。

@AppVeyorBot
Copy link

@sonarcloud
Copy link

sonarcloud bot commented Mar 3, 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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@berryzplus berryzplus marked this pull request as ready for review March 3, 2021 17:04
@AppVeyorBot
Copy link

@beru
Copy link
Contributor

beru commented Mar 3, 2021

戻り値の型を本来は size_t にするべきだと思いますが、そうすると呼び出し元で受け取る型を変えたりしないといけないので収拾がつかなくなるんですよね。

@berryzplus
Copy link
Contributor Author

戻り値の型を本来は size_t にするべきだと思いますが、そうすると呼び出し元で受け取る型を変えたりしないといけないので収拾がつかなくなるんですよね。

ええ、x64対応的には size_t にするのが自然だと思います。

しかし、それは #524 でやろうとしてた感じの大規模修正になります。
ただ、修正規模の割りに対応メリットが薄い気がするのでビミョーな感じになっちゃいます。

戻り値をsize_tにするメリットは以下かと思います。

  1. 戻り値が負数である場合の考慮が要らなくなります。 👈やったほうがいいかも。
  2. INT_MAX を超える個数を返せるようになります。 👈やらなくていい気がします。

INT_MAXの値は約20億で、サクラエディタが同時に開けるファイル数は50個くらい(設計上は100くらい)です。

なので、値域を拡大する意味はないように思います。

@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit 8403d1e into sakura-editor:master Mar 4, 2021
@berryzplus berryzplus deleted the feature/cast_to_int branch March 4, 2021 03:27
@berryzplus berryzplus added refactoring リファクタリング 【ChangeLog除外】 x64 x64 対応 labels Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】 x64 x64 対応
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants