-
Notifications
You must be signed in to change notification settings - Fork 168
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
[WIP] ソースコードの整形の実験 #1172
[WIP] ソースコードの整形の実験 #1172
Conversation
bd3fd40
to
d32aacd
Compare
✅ Build sakura 1.0.2568 completed (commit d845635630 by @m-tmatma) |
今ある自動フォーマッタはまだまだこういうのを考慮して良い塩梅で調整はしてくれないと思うので、あーあ…って感じです。とはいえ人によっては、そんな些細な事より自動フォーマットで得られる価値の方が大きい、と思うかもしれません。 |
d32aacd
to
8464b08
Compare
✅ Build sakura 1.0.2570 completed (commit 559106f23a by @m-tmatma) |
サクラエディタのソースコードに特有なフォーマット
C++的な標準はたぶん、
やってしまってもいいと思いますが、#1172 (comment) の問題があるのでドキュメントフォーマットでやるのは辛いのかな、と思いました。 javaというか、Eclipseにはソースコードの自動フォーマットを一部除外する機能があります。 Visual Studio に同等の機能があれば、それを活用することも検討したいです。 |
8464b08
to
f529bc1
Compare
✅ Build sakura 1.0.2573 completed (commit cfe0911b54 by @m-tmatma) |
f529bc1
to
a7aa935
Compare
現在の状態 fileNo → 指定フォルダ内の cpp/h ファイルの数
|
✅ Build sakura 1.0.2574 completed (commit b5be883d09 by @m-tmatma) |
Visual Studio で指定範囲をフォーマット対象から外す方法は分かりません。
// clang-format off
フォーマット対象から外したいコード
// clang-format on 大量のコードを手動で特定のスタイルに修正するのは大変で普通は手動でやりたがらないだろうと思います。なので、一部のコードの見た目が崩れるとしてもそういうのには妥協というか目をつぶって自動フォーマッタを使いたいという考えもまぁ分からなく無いです。つまるところ人によって論理や感覚や何を優先するかの好みって色々だと思うので。 ある時点でスタイルを統一したとしても、その後に誰かがPRで変更を入れるたびに細かく指摘したり、あるスタイルに則るように強制したりとかがないと良いなぁと思います。常に特定のスタイルで統一させたい人もいるかもしれないですけど。:sweat_smile: |
何故、差分が出るのか?って、検証してます? |
解析は途中です |
ただ週末まで再開できないです |
主にデバック版で差分が出てます |
機能的な差異が出ないことの確証がとれてからの話になりますが、 というか、CIにチェックスクリプトを組み込む流れになる予想です。 だって、目で見て指摘するのめんどくさいじゃないですか(色んな意味で。 |
了解っす。気が向いたらこっちでも調査してみます。(たぶんやんないw
ヘッダをインクルードする順番によって使われる 「ヘッダーの依存関係が適切でないかもしれない」って予想がアタリだとすると何気にやっかいな課題になるかもです。 |
CIのチェックスクリプトでフォーマッタ適用結果と差分が出た際にPRのレビューワーがどう判断するかは人それぞれかもしれません。ですが、引っ掛かってるから直してちょ的な指摘を誰かがして解消されない限りApproveしない場合それって最早暗黙のルールみたいなものではないかと思います。とはいえ個々のレビューワーの内々の判断基準にまで口出しするのってそれこそどうなの?とか言われたら、はいそうですね、って感じになりそう。 コーディングスタイルにルールは決めないっていう方針みたいのが有った気がしますが結局は、絶対的なものは絶対にない、的な感じで近い内になし崩しになりそうですね。 |
この件のownerは @m_tmatma さんなので、実験の結果どうするかの判断は投げる感じです。 コーディングスタイルの策定は、みんなが迷わずにコーディングをするために必要なものです。 通常のプロジェクトではアーキテクチャ策定の一環として行います。大きなプロジェクトを成功させるには、作る前にルールを決めるのが必須です。 既に完成しているサクラエディタで決め直すのは本来的には「変」だと思うんですけどね。 ルール決めないのはルールじゃないはず。 単に決められなかったの後付けで決めないって説明つけただけなので、必要なら新たに決め直したらよいと思います。 サクラエディタを建造物に喩えると、ちょっと豪華な民家風の民宿だと思うんです。 これを、豪華な今風のリゾートホテルに建て替えたいって話が出ています。(普通に考えたら不可能です。 やろうとしてるのはそういう作業だと思っとります。 |
a7aa935
to
21546e8
Compare
✅ Build sakura 1.0.2578 completed (commit 5e3b0cf26b by @m-tmatma) |
21546e8
to
e9ce55e
Compare
✅ Build sakura 1.0.2579 completed (commit 57110deadb by @m-tmatma) |
✅ Build sakura 1.0.2580 completed (commit 3e8257ea10 by @m-tmatma) |
fb6239f
to
dc9a97b
Compare
単に 行番号( LINE ) がズレる話だったみたいですね。 |
✅ Build sakura 1.0.2584 completed (commit d1a8a2b6e4 by @m-tmatma) |
この課題に対する原因が以下。 sakura/sakura_core/debug/Debug2.h Lines 41 to 55 in b3278eb
ソースコードに行番号( いったんソースコードに行番号が入らないようにしてる作業の途中・・・。
|
説明欄を更新しました。 |
これってなんでしたっけ?
どっちかだと思いますが、いずれにしろ理由説明が必要だと思います。 理由があるなら「代わる」じゃなくて「変える」ですよね。 |
です。 |
これな。 msvcのメモリ確保関数には、トレース機能が付いていて、解放漏れが検出されたときにメモリ確保が行われたファイル名&行番号を報告する仕様になっているわけです。 フォーマッタの適用で行番号がずれると当然そこに差分に出てしまうので、 |
何故だっ!www vs2017で普通に新規関数を書くと、関数本体は
これの実験の話とは別に、どんな設定でフォーマットするのか?を検討せにゃならん気がします。 どっかに標準があればいいけど、たぶんないのでvs2017クリーンインストール時の設定とかに盲目的に追従することになるのかな、と思っています。 |
私の環境でもそうです。 |
https://clang.llvm.org/docs/ClangFormatStyleOptions.html で
で .clang-format を生成して
というように整形してみましたが、 |
.editorconfig をみているのかもと思ったが、削除しても同じだった。 |
ん?やってるはずの操作を手元で検証した結果とずれてそうです。
この手順の後にサクラエディタで |
話の流れを忘れとりました。 一括フォーマット適用後にそれと異なるスタイルのPRがなされたらどうするか?について、「許容で良い」の見解は変わってません。 ただし、コーディングスタイルが他と異なっていたら意図は確認すると思います。 めんどくさいですが、そういうどうでもいいことを含めて「何故そう変えるのか」を明らかにし、第三者視点から変更の妥当性を検証するのがレビューだと思うので。 コード開発をある程度の多人数で行い、かつ、個々のプログラマーの成熟度があまり高くない場合、コーディング規約の策定は必須だと思います。 可能な限り「アフォでも参加できるプロジェクト」としていきたいのでコーディング規約があったほうがいいと思っています。 まとめ |
別の PC でやった場合、インデントはタブになったのですが、 何か visual studio 側の設定が違うのか、バージョンが違うのか、何かプラグインが
のところが担保できるのであれば、 clang-format.exe で整形してしまいたいですが。 |
Visual Studio の Text Editor > C/C++ > Formatting の設定は人によってはカスタマイズして使う事があると思いますが、設定ファイルか何かを自動で読んでサクラエディタのプロジェクトの場合に決まった設定を使うという事であれば運用しやすいかなと思います。VSで扱うプロジェクトを変える度にウィザードを操作して設定をインポートし直すのは面倒だし出来ればやりたくないです。 |
コーディングスタイルを揃えた後の話ですが、その後のPRで特に変える必要がないのにコーディングスタイルだけ変えてるような変更が有った場合は、あえてツッコミを入れるのは有りだと思います。ただスタイルを統一している事を関知していない誰かが新規で1000行とかのPRを作成した場合に、コーディングスタイルが既存のものと違うからといって補導 & 査問 & 尋問とかは止した方が良いと思います。
コーディング規約を決めた方が良いと思う理由って人によって様々かもしれないですね。 そもそもは @m-tmatma さんが Visual Studio のエディタにペーストした際にコードが勝手に整形されるけどそれで差分が出るのが嫌だから、って事が発端ですよね? sakura-editor/management-forum#29 (comment)
自分の意見としてはそれが嫌ならVSの設定を変えれば良いじゃないかと言ったら、 差分がいちいち出るのが嫌という個人的な問題への対処方法として設定を変えたらどうか?と聞いたら、それはソースコードを編集したい人すべてに対してルールを導入する事になると言われました。なんか話が噛み合わない気がしますが…。
@m-tmatma さんとしてはコーディング規約を策定したいわけではなくあくまで差分が生じないように特定のスタイルにしたいのかもしれません。なので、ルールを導入するわけではないと表明したのかもです。 ただ今の @berryzplus さんとのコメントの流れだとコーディング規約は策定するという方向に行っているような気がします。 インデント位置とか括弧の前後の半角スペースの有無が揃っていないと気になるのでコーディング規約を導入するのもまぁ良いんじゃないかとも思います。自分好みだと良いですがまぁそうでなくても多少は我慢は出来ます。 ただ統一させる事に腐心するのってどうなのかなぁって気もしちゃいます。今のままじゃ駄目なんですか?みんなの好みも色々だろうし…。 |
結局、コーディング規約を策定する「必要があるのか?」というと「ない」と思っています。 コーディング規約が必須なプロジェクトの条件として2つ書きました。
サクラエディタのプロジェクトに関して「どないやねん!」と考えた場合、 なので現状、コーディング規約は不要です。 もちろん、ずっとこのままで良いか?と考えたら違う見方になってきます。
現状のプログラミングスタイルに関する合理的な理由の考察。 たぶんこういうことだと思います。 if( conditionA == true &&
conditionB == true ){
// ↑インデントにより条件の開始位置が揃っている 現代基準では、改行位置は短絡ANDの「前」とするのが定石 だと思います。(もちろんどちらでも良いですが、書籍のスタイルガイドに明記される指針では行頭派が多い気がします。 短絡ANDや短絡ORの位置を行末とする、な規約が存在するのであれば、合理的なスタイル指針だといえなくもないです。 Visual Studioのエディタ設定を変えて現状の規約(?)に合わせる試み。 無理でした。詳細 現状にあわせてみようとして無理だったから、現状を標準(?)に合わせてしまおうとしてる感じです。 なんかちょっといま、迷走してる感じですけど 😄 |
このPRでやってることって、主にデバッグビルドのasm出力が変わることへの対策ですよね・・・。 視点を変えて「フォーマット適用前後の変更なし確認はリリースのasmで行う」としたら、テストコードを適用する必要はなくなるし、挿入したテストコードを確実に除去するための作戦を考える必要もなくなるような。 もちろん、リリースビルドにしたら差分が出ないかというとちょっと違くて、意図的に行番号を挿入している箇所は差分になりますよね。そんなに数は多くないと思うので、そこは個別に確認でよいような気がします。個人的には差分を出さないためにテストコードを入れて戻し忘れるリスクが怖いっす。 まぁそれ以前に「どうやってフォーマットを適用するか?」に課題がある感じ(=環境によってフォーマット結果が変わってしまう)なので、そこを解決するのが先決ですかね。 |
今回試行錯誤で試したのでコミットを分けていますが 今回でも、コミット等にログに debug用と入れているので |
#1172 (comment) の原因が分かりました。(確定でもないんですが(^^;
フルパスは以下です。 インストールされているバージョンは6.0っぽいです。 C:\Users\berryzplus> "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\vcpackages\clang-format.exe" -version
clang-format version 6.0.0 (tags/RELEASE_600/final)
C:\Users\berryzplus> cd C:\work\sakura-editor
C:\work\sakura-editor> "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\vcpackages\clang-format.exe" -dump-config
Language: Cpp
...(省略)
Standard: Cpp11
TabWidth: 8
UseTab: Never なんとなく分かると思うんですが、「UseTab」は「タブを使うか」を設定する項目で、「Never」は「使わない」です。 え?マジで? と visual studio 2017 の
設定ファイル置いてね、って書いてあるやん・・・。 というわけで、 |
ん?「
|
追加情報(vs2019) C:\work\sakura-editor\sakura>"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\Llvm\bin\clang-format.exe" -version
clang-format version 9.0.0 (tags/RELEASE_900/final)
C:\work\sakura-editor\sakura>"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\Llvm\bin\clang-format.exe" -style=Microsoft -dump-config
---
Language: Cpp
# BasedOnStyle: Microsoft
AccessModifierOffset: -2
...(省略)
Standard: Cpp11
StatementMacros:
- Q_UNUSED
- QT_REQUIRE_VERSION
TabWidth: 4
UseTab: Never 出力全文
|
PR の目的
[WIP] ソースコードの整形の実験
カテゴリ
PR の背景
sakura-editor/management-forum#29
で ソースコードの整形をすることに関して議論したが結論が出なかった。
試しに整形してみて、議論のネタにする。
PR のメリット
Visual Studio が勝手にソースコードを整形しても不要な差分が出なくなる。
PR のデメリット (トレードオフとかあれば)
既存の PR がコンフリクト祭りになる
インデントがタブからスペースに代わる
注意
アセンブリ出力を確認するために、一部テストコードを入れている
(Debug用)
とつけているコミットを入れることで一致させることができた。#line
で行番号を固定ビルド
PR の影響範囲
関連チケット
参考資料