Skip to content

Conversation

@kengoide
Copy link
Member

@kengoide kengoide commented Apr 2, 2021

PR の目的

stdafx.h からプロジェクト内のファイルを可能な限り除去し、コード修正後のビルド時間の削減を狙います。

カテゴリ

  • リファクタリング

PR の背景

サクラエディタのコードについて以前から指摘されてきた事柄の一つとして、プリコンパイル済みヘッダーの使用方法についての疑義があります。

プリコンパイル済みヘッダーは通例めったに内容が変わらないファイルを先にコンパイルすることでビルド時間を削減します。プリコンパイル済みヘッダーに含まれるファイルはプロジェクト内のすべての翻訳単位からの依存関係が生じるため、不適切なファイルをインクルードすると却って生産性を削ぐ結果となります。これを踏まえた適切なインクルード対象は標準ヘッダーや外部ライブラリのヘッダーであり、プロジェクト内の共通ヘッダーなどよく使うファイルをインクルードすることは誤用であるという指摘です。

この PR は stdafx.h からプロジェクト内のファイルの大部分を除去するものです。stdafx.h のありかたについての合意が形成されているか定かではないことから、現状では Draft としています。

PR のメリット

  • ヘッダーファイルを修正した際のビルド時間が短縮できる可能性があります。
  • SonarScanner のキャッシュ機能の時短効果が向上します。

PR のデメリット

  • リビルド時間がわずかに長くなります。
    • 手元の環境では全体リビルド時間が1秒増加しました。

仕様・動作説明

プロジェクト内のヘッダーファイルのすべてを除去する予定でしたが、env/DLLSHAREDATA.h に限り存置しています。リビルド時間が大幅に悪化したため妥協せざるを得なくなりました。

PR の影響範囲

テスト内容

  • PR適用後にビルドエラーが発生しないことを確認する。
  • リビルド時間が大幅に悪化していないことを確認する。

バイナリのテストは不要だと思います。

参考資料

VS2019/C++: stdafx.h stdafx.cpp または pch.h pch.cpp つまり「プリコンパイル済みヘッダー」( Precompiled Headers ) の使い方のメモ

kengoide added 28 commits April 3, 2021 00:26
@kengoide kengoide force-pushed the feature/thinner-stdafx branch from 9754249 to 57872ee Compare April 2, 2021 15:32
@usagisita
Copy link
Contributor

自分は「誤用である」かについての議論に参加する気はないですが、削除、削減することに関しては、賛成です。
CNativeW.hなど重要ファイルへの変更で、リビルドをする頻度が高まっているとも感じています。
逆に言えばstring_viewとかは新たに入れておいたほうがいいかもしれません。
無事コンパイルが通れば、処理が以前と異なってバグが増えるということも、まずないので僕個人は反対する理由はありません。

@AppVeyorBot
Copy link

Build sakura 1.0.3632 failed (commit 3ed2b7ab0d by @k-kagari)

@AppVeyorBot
Copy link

Build sakura 1.0.3633 failed (commit 8521a75eab by @k-kagari)

@kengoide kengoide force-pushed the feature/thinner-stdafx branch from fd7a41c to e42b45b Compare April 2, 2021 16:21
@AppVeyorBot
Copy link

Build sakura 1.0.3634 completed (commit c21021e534 by @k-kagari)

@beru
Copy link
Contributor

beru commented Apr 2, 2021

リビルドにかかる時間は自分の環境では master (0de7f4e) と feature/thinner-stdafx (e42b45b) のどちらも13秒ぐらいでした。

@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Apr 3, 2021
@AppVeyorBot
Copy link

Build sakura 1.0.3637 completed (commit 14ce7eafe1 by @k-kagari)

@kengoide
Copy link
Member Author

kengoide commented Apr 3, 2021

肯定的なお言葉をいただいていますので Draft を外します。

静的解析の警告についてはほぼすべて既存の問題と思われます。

差分が巨大でレビューが難しいかもしれませんが、大部分は #include や前方宣言を追加する機械的な変更です。stdafx.h の差分を重点的に見ていただけると助かります。

@kengoide kengoide marked this pull request as ready for review April 3, 2021 11:43
@berryzplus
Copy link
Contributor

berryzplus commented Apr 3, 2021

プロジェクト内のヘッダーファイルのすべてを除去する予定でしたが、env/DLLSHAREDATA.h に限り存置しています。リビルド時間が大幅に悪化したため妥協せざるを得なくなりました。

stdafx.hの該当部分にその旨コメントしておいたほうが良いと思います。

例👇

// 以下はプロジェクト内のファイルだがプリコンパイルしておく。
// (ビルドパフォーマンスが著しく悪化するため。)
#include "env/DLLSHAREDATA.h"

パフォーマンス悪化の原因は「ヘッダー依存関係が不適切だから」だと思っています。
「マテ」というツッコミは「なし」の方向で。

berryzplus
berryzplus previously approved these changes Apr 3, 2021
@kengoide
Copy link
Member Author

kengoide commented Apr 3, 2021

stdafx.hの該当部分にその旨コメントしておいたほうが良いと思います。

コメントを追加しました。

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 3, 2021

SonarCloud Quality Gate failed.

Bug E 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 26 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3638 completed (commit 59dc358634 by @k-kagari)

@kengoide kengoide merged commit df7c2cd into sakura-editor:master Apr 3, 2021
@kengoide
Copy link
Member Author

kengoide commented Apr 3, 2021

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

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.

5 participants