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.

Sign up for GitHub

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

Jump to bottom

ソースコードのスタイルに関して #29

Open
m-tmatma opened this issue Sep 8, 2018 · 30 comments
Open

ソースコードのスタイルに関して #29

m-tmatma opened this issue Sep 8, 2018 · 30 comments

Comments

@m-tmatma
Copy link
Member

m-tmatma commented Sep 8, 2018

サクラエディタのソースコードで visual studio の標準のスタイルとは異なるスタイルが
使われている箇所がたくさんあります。

例えば以下のコードの部分を選択して、カット & ペーストすると visual studio が
visual studio の標準のスタイルで整形します。

https://github.com/sakura-editor/sakura/blob/82724c0295f3559c3c6bcf2df34a76ba05b5170b/sakura_core/view/CEditView_ExecCmd.cpp#L126-L141

以下は visual studio が自動的に整形したコード

	ECodeType outputEncoding;
	if (nFlgOpt & 0x08) {
		outputEncoding = CODE_UNICODE;
	}
	else if (nFlgOpt & 0x80) {
		outputEncoding = CODE_UTF8;
	}
	else {
		outputEncoding = CODE_SJIS;
	}
	ECodeType sendEncoding;
	if (nFlgOpt & 0x10) {
		sendEncoding = CODE_UNICODE;
	}
	else if (nFlgOpt & 0x100) {
		sendEncoding = CODE_UTF8;
	}
	else {
		sendEncoding = CODE_SJIS;
	}

このように visual studio の標準のスタイルと違うスタイルで書かれているコードは
visual studio が自動的に整形するので、コードの場所が近い場所を修正したときに
必要のない修正が入ってしまうことになります。

それで、レビューしやすいように、必要のないスタイルの差分ができないように
手動でwinmerge とかで確認しながら、戻してコミットしています。

しかし、この作業は結構不毛なので、その作業が必要のないようにしたいと思います。

どこかのタイミングで UTF-8 変換でやったようにフォルダを選びながら
すべてのソースコードを visual studio の標準のスタイルに統一したいです。

どう思いますか?
@m-tmatma
Copy link
Member Author

m-tmatma commented Sep 8, 2018

https://github.com/sakura-editor/sakura/blob/82724c0295f3559c3c6bcf2df34a76ba05b5170b/build.md#githashh-%E3%81%AE%E6%9B%B4%E6%96%B0%E3%81%AE%E3%82%B9%E3%82%AD%E3%83%83%E3%83%97

に記載していますが、環境変数 SKIP_CREATE_GITHASH を 1 にしてビルドすることにより
githash の生成をスキップできるのでスタイルの修正のみなら、asm ファイルの完全一致を
確認することにより、デグレがないことを保証できます。

@beru
Copy link

beru commented Sep 8, 2018

Visual Studio の設定を変えて、ペースト時に自動的に整形しないようにするのはいかがでしょうか？

@beru
Copy link

beru commented Sep 8, 2018

どこかのタイミングで UTF-8 変換でやったようにフォルダを選びながら
すべてのソースコードを visual studio の標準のスタイルに統一したいです。

Visual Studio の標準のスタイルに統一、というのは自動化出来るのでしょうか？

どこかのタイミングで何かのスタイルに統一する事自体は反対しません。
しかし仮にコーディング規約とかを作って特定のスタイルに常時強制するとなると抵抗感があります。

どうしてそう考えるかというと他のプロジェクトでの経験なのですが、 .clang-format ファイルが用意されてて Travis CI でシェルスクリプトでチェックして特定のスタイルに沿っていなかったらエラーとするように自動化されていたんですが、PR出す人がそれが分からなくてレビューワーが指摘する、みたいなのばっかりで。。まぁこのプロジェクトでは大丈夫なのかもしれませんが。

あと特定のコーディングスタイルを推奨する人がいつまでも活発にメンテしてくれるかというとそうとも限らないと思うので、PR出す人の負担につながるルールを導入するのは慎重になった方が良いと思います。

@berryzplus
Copy link

ソースコード開いて Ctrl + K, Ctrl + D とするとドキュメント全体がオートフォーマットされます。

コーディングスタイルの設定は vs2013 とか vs2015 とかの設定が自動同期されたはずなので、
何をもって「デフォルト設定」とするかは事前調整が要る気がします。
なんかの手順で「vs2017 のデフォルト」に戻せるはずですが、やり方を知らんので。

@beru
Copy link

beru commented Sep 8, 2018

ソースコード開いて Ctrl + K, Ctrl + D とするとドキュメント全体がオートフォーマットされます。

これは編集しているファイル単体が対象のようですね。

Extension の Macros for Visual Studio 入れれば複数ファイル一括で出来るかもしれませんが、そうしないでもコマンドで楽に出来る方法があると良いですね。

なんかの手順で「vs2017 のデフォルト」に戻せるはずですが、やり方を知らんので。

メニューの ツール(&T) > 設定のインポートとエクスポート(&I)... を選択すると、設定のインポートとエクスポート ウィザード 画面が表示されるので、そこで すべての設定をリセット を選択すると初期値に戻せるみたいです。多分これでいけるんじゃないでしょうか。

@m-tmatma
Copy link
Member Author

m-tmatma commented Sep 8, 2018

何かルールを導入するというわけではなく、何も設定せずに visual studio で編集したときに
visual studio が気を利かせて? スタイルを調整することで差分が出てしまうのであらかじめ
そのスタイルにしたいということです。

しかし仮にコーディング規約とかを作って特定のスタイルに常時強制するとなると抵抗感があります。

コーディング規約を作るわけではないです。
現状のコードで visual studio がお気に召さないコードをvisual studio の好みにそろえるだけです。

visual studio がデフォルトで何も整形しないのであれば現状でもいいのかもしれませんが、
デフォルト設定で使ったときに問題が発生しにくくするのがとりあえず目標です。

Visual Studio の設定を変えて、ペースト時に自動的に整形しないようにするのはいかがでしょうか？

これだと、ソースコードを編集したい人すべてに対して、Visual Studio の設定をあらかじめ
設定してください、というルールを導入するということになります。

これは編集しているファイル単体が対象のようですね。

現在編集しているファイルの場合は、オートフォーマットが可能なのは確認しています。
複数ファイルで可能なのかは、実行したいというのが決まったら検討しようと思います。

@KENCHjp
Copy link
Member

KENCHjp commented Sep 8, 2018

以前、別Issueでかいたと思いますが、コーディングスタイルを決めて「人が」ルールを守るのは、
本来の目的以上に体力を使うので、特段だれかに納品したりするものじゃないから、決めない方向にしようと書いたと思います。

が、 @m-tmatma さんがおっしゃってるのは、VSが勝手にあらぬところも変えちゃうのでそれで差分が出ないように、手で修正しているということで、それを避けようってことかと思うので、
それは賛成です。

ペースト時に自動整形しない設定もあるかと思いますが、それだと本当にコーディングするときに各々のコーディング労力が著しく低下したりするのかなと思います。

こんごPRされたソースに対してどうするねんてことかと思うのですが、結局労力の差さと思うので、がちがちにルールを決めずに適宜実施でいいのかなと。

@beru
Copy link

beru commented Sep 9, 2018

これだと、ソースコードを編集したい人すべてに対して、Visual Studio の設定をあらかじめ
設定してください、というルールを導入するということになります。

設定を変えなければいけない ルール ではなく、設定を変える事で問題を回避するのはどうでしょうか？という 提案 です。

こういう事が生じないようにMSが最初から、貼り付け時にフォーマットするかしないかでコマンドを別々にするか、フォーマットしない貼り付けコマンドを用意してくれていれば良かったのにと思いますが…。

Visual Studio の設定（テキストエディター > C/C++ > 書式設定 以下）を自分好みにカスタマイズしてる人も中にはいると思いますが、そういう人達が同様にペーストした場合に（そしてペースト時に自動フォーマットする設定を有効化していて）結局Visual Studio の標準のスタイルと比べて差異が生じる事にならないでしょうか？

で、そういう人が @m-tmatma さんと同じ差分が生じる事による悩みを持った場合は、設定を標準に変えるのがルール推奨される解決策なのでしょうか？そういう人達が N 人いたとして、Issue が N 個生じたり、スタイル統一PRが N 個不定期に発生する事は現象として頭おかしすぎるので無いと思いますが。

スタイル統一などで差分が大量にあるコミットが必要に応じて生じてもまぁしょうがないと思えるので、されたい事に対して反対しているわけではないですが、人次第で変わってしまう事への解決として統一して周りにも足並みを揃えてもらう、という解決策を取らざるを得なくなるというのはなんかすっきりしないなぁと。。Visual Studio に途中で入ってデフォルトで有効になってしまった機能に振り回されているようで。

別の似たような例ですがセキュア版の関数を使わないと Warning が出るのが標準仕様になったりと（セキュリティホールが頻発しすぎて痛い目にあったからかもしれませんが）開発環境がユーザーに対して半強制的に選択肢を押し付けてくるのも好きではありません。

で、自分の意見としては、コミット作ってまで足並みを揃えるよりかは、エディタの設定を変えれば済む話なのだから変えれば良いのではないかと思います。

ペースト時に自動整形しない設定もあるかと思いますが、それだと本当にコーディングするときに各々のコーディング労力が著しく低下したりするのかなと思います。

それだけで各々のコーディング労力が著しく低下したりするとしたら、お前らどんだけVSに飼いならされてるんだよ…って思います。確か VS2015以降から入った機能だと思いますが習慣って怖いですね。ペースト時に自動整形しない設定にしても後からドキュメントや選択範囲を整形するコマンドがあるので、それを使っていただけないでしょうか。。

差分管理に労力費やさないといけないのは自分も日々経験してますが、@m-tmatma さんが行われている操作内容を同じ設定の環境で行っているわけでは無いので共感出来ていないだけかもしれません。

@KENCHjp
Copy link
Member

KENCHjp commented Sep 9, 2018

お前らどんだけVSに飼いならされてるんだよ…って思います。

私は、C++側には貢献できていないので、どちらがより楽なのかは全く根拠レスです。
問題点と「コーディング規約を定めるわけではない」という点は共有できているかと思うので、
あとは手段ですよね。

今出てる意見「設定を変える→設定の強要につながる、設定変えない人の差分は出てづける？」「ソースを変える→今後デフォルトが変わったら毎回全ソースコンバートする？」
こんな感じですかね。

個人的には、全ソースオートフォーマットするのはあんまりやりたくないですねぇ（仕事だと全プログラム再テストのパターン）。

コンパイルスイッチとかは合わせないとヤバそうですが何か不都合があったら、プロジェクト固有でコンパイルスイッチ等規定が必要な場面の出てきそうなので、なにがしかの設定を強要する（設定しない人も即なにか不都合が起きるわけではない、差分の取り込みが面倒かもしれないが）のも致し方ない気もしてきました。

@berryzplus
Copy link

どっちかなぁ・・・言ってることはどちらも正しい気がしました。

  • 変な差分が出るのがあらかじめ分かってるのだから対策したい
  • 差分の原因になるvsの変更は将来にわたって発生するのだからいちいち対応する必要はない

個人的には「変な差分」が出るのがめんどくさいです。
対応としての一括フォーマットは「あり」だと思います。

サクラエディタのソースコードは700ファイルくらいあるので、
ユニコードのときみたく人力でチェックするのはしんどいです。

半自動で整形できる手順を用意して手順自体を検証し、
2人以上で同じ結果になることを確認できれば「それを入れる」でいいのかな、と思ってます。

@berryzplus
Copy link

あれ？ソースコードの変更が処理内容を変えてるかどうかを検証する枠組みはありますよ。
処理内容が変わってたらアセンブラが変わるので、そこで検出できます。

@kobake
Copy link
Member

kobake commented Sep 9, 2018

全整形に1票です。VSデフォ設定の人に優しい環境が良いです。影響はアセンブリ比較でイケるかと。

@KENCHjp
Copy link
Member

KENCHjp commented Sep 9, 2018

あれ？ソースコードの変更が処理内容を変えてるかどうかを検証する枠組みはありますよ。

なるほど、そうですね、まえやりましたね。
会社（仕事）では、この手段広めるようにしよう。昔はそういう発想なかったなぁ。
コンパイル時になにがしかタイムスタンプ的なものが変わったりするとバイナリには必ず差分でてきてたとおもうので。

@kobake さんも１票入ったので、こっちっすかねぇ。私はコーダーではないので死に票かと(笑)
@berryzplus さんいわれるようにどっちがどうって感じでもない気はしています。より今後工数が楽な方は場面によっても変わってくるかもしれませんし。

@m-tmatma
Copy link
Member Author

m-tmatma commented Sep 9, 2018

実現方法に関して

FormatDocumentOnSave というプラグイン

VSE-FormatDocumentOnSave という visual studio プラグインがあって、現在開いているドキュメントに対して Edit.FormatDocument のコマンドを実行して整形するということができるようです。

https://github.com/Elders/VSE-FormatDocumentOnSave
https://marketplace.visualstudio.com/items?itemName=mynkow.FormatdocumentonSave

仕組み

VisualStudioCommandFormatter というクラスの Format を呼ぶと DTE.ExecuteCommand で
Edit.FormatDocument を実行することにより実現しているようです。

https://github.com/Elders/VSE-FormatDocumentOnSave/blob/babfdbd2b3530e647d3be6896302fb947104dd02/src/Elders.VSE-FormatDocumentOnSave/VisualStudioCommandFormatter.cs#L19-L36

実現方法

  • (案) 同様の仕組みで複数ファイルを同時に処理できるプラグインを探す。
  • (案) 同様の仕組みで複数ファイルを同時に処理できるプラグインを作る。

@ds14050
Copy link

ds14050 commented Sep 9, 2018

一括整形した後はどういう運用を考えていますか。

「ある時点の VS によるフォーマット」を強制しては人に優しくありません。自動整形による差分がいったんゼロになるが、その後ソースに手を入れるにつれ微増していくことを許容してもらえるのでしょうか。

@KENCHjp
Copy link
Member

KENCHjp commented Sep 9, 2018

その後ソースに手を入れるにつれ微増していくことを許容してもらえるのでしょうか。

ここは、がちがちにしないルールは共通認識かと思うので、またPr時に気づいたときにそっと修正するでもいいのかなと思うのですが。
毎週のようにリフォーマットなんて運用してると、リポジトリがそれだけで、大変なことになるのかも。
それとも毎PR走るごとに整形する？？？

@kobake
Copy link
Member

kobake commented Sep 9, 2018

許容で良いかと。

@berryzplus
Copy link

同じく、許容でよいと思います。

面倒なのはノイズが「ある」ことじゃなくて、ノイズが「多い」ことだと思ってるので。

@m-tmatma
Copy link
Member Author

m-tmatma commented Sep 9, 2018

https://blogs.msdn.microsoft.com/vcblog/2018/03/13/clangformat-support-in-visual-studio-2017-15-7-preview-1/

clang-format.exe で整形しているのかと思ったが、

By default, Visual Studio is selected, meaning we do what Visual Studio usually does for formatting and do not run ClangFormat at all.

デフォルトでは clang-format.exe は使わないらしい。

@berryzplus
Copy link

clangformat とは別に msvcformat がありそうな書き方ですね。
clang は apple と google の共同プロジェクトだからunix系c++なのかな？
想像ですが、vcのフォーマットには vcXXX.dll が使われてるんじゃないかと思います。

@beru
Copy link

beru commented Sep 9, 2018

使った事無いですがこんなのがありました。
https://marketplace.visualstudio.com/items?itemName=munyabe.FormatAllFiles

@berryzplus
Copy link

使った事無いですがこんなのがありました。
https://marketplace.visualstudio.com/items?itemName=munyabe.FormatAllFiles

むぅ...。ダウンロードして検証中です。
ちゃんと動くなら正に評価コメントにある通り。

This was exactly what I was looking for.

@berryzplus
Copy link

イケてそうです・・・

変更されるファイル数 654でした。

ソリューションの右クリックからでは正常動作しない点、
自動生成ファイルが含まれているとFileNotFoundになる点、
フォーマットに対応してないファイル(.txt等)が含まれると「Error」と報告される点、
ダメ出しポイントは結構ありますが、とりあえず使えそうです。

影響範囲の広いPRがいくつか上がってるので、実施タイミングは別途、と考えています。

berryzplus added a commit to berryzplus/sakura-editor that referenced this issue Sep 9, 2018
@berryzplus
フォーマットツールでコード整形
dbc948c 
https://marketplace.visualstudio.com/items?itemName=munyabe.FormatAllFiles
sakura-editor/management-forum#29 (comment)
@berryzplus
Copy link

フォーマッタ試した結果を上げときます。
https://github.com/berryzplus/sakura/tree/eval_format_all
（そのうち消します）

@k-takata
Copy link
Member

k-takata commented Sep 9, 2018

フォーマッタ試した結果を上げときます。

ちょっと見てみましたが、caseの中の {} をインデントしないのは非常に違和感を感じます。
berryzplus/sakura-editor@dbc948c#diff-440be1bd3c37411f5e6500dcbcb1695fR191

	}
	break;
	}

こんなインデント、自分のコードなら不許可です。

@berryzplus
Copy link

それは確かに嫌ですｗ

フォーマット設定は弄ってないつもりですが、もしかしたら自分が触った影響かもしれません。
一応、フォーマット後のコードには手をつけていません。
実際の適用はデフォルト設定にリセットして行う話の認識です。

とりあえず上げてみた意図としては、
　こんなにたくさん変更があるとか
　一律変更だとこんな感じになる
を示すことです。

何も問題なく・・・とはいかないのかなぁ。

@beru
Copy link

beru commented Sep 9, 2018

何も問題なく・・・とはいかないのかなぁ。

ファイルヘッダのコピーライトの文章に、

3. This notice may not be removed or altered from any source distribution.

と書かれていますが、3行ほど行末の余分な空白が削られる 変更 がされているので通報しますた。

@beru beru mentioned this issue Jan 25, 2020
Merged
@m-tmatma
Copy link
Member Author

sakura-editor/sakura#1170
で言及されていて思い出してこのチケットを見てみましたが、

どうやって実現するのかで止まっていたと思います。

#29 (comment)

ソースコード開いて Ctrl + K, Ctrl + D とするとドキュメント全体がオートフォーマットされます。

Edit.FormatDocument の機能に Ctrl + K, Ctrl + D のキーボードが割り当てられているので
何か一つのキーに割り当てて、愚直に全ソースに対して実行していけばいいんじゃないかと
思うのでやってみようと思います。

@m-tmatma
Copy link
Member Author

Visual Studio 以外のフォーマッタを使うといろいろ問題みたいですが、
Visual Studio でやってみます。

@m-tmatma m-tmatma mentioned this issue Jan 26, 2020
Closed
@m-tmatma
Copy link
Member Author

sakura-editor/sakura#1172
を作成してみました。

berryzplus added a commit to berryzplus/sakura-editor that referenced this issue Mar 29, 2020
@berryzplus
一括フォーマットのサンプル
531400e 
vs2019 + .clang-format でフォーマットしたもの。
一度だけ手動でCtrl+K,Ctrl+Dしてからvsixで一括置換。
sakura-editor/management-forum#29 (comment)
@berryzplus berryzplus mentioned this issue Mar 29, 2020
Closed
This was referenced Sep 6, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@k-takata @beru @kobake @berryzplus @m-tmatma @ds14050 @KENCHjp