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

assert を独立した 1 行で記載する #1217

Merged
merged 3 commits into from
Mar 21, 2020

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Mar 20, 2020

PR の目的

assert を独立した 1 行で記載する

カテゴリ

  • リファクタリング

PR の背景

#1179, #1172 で アセンブリ出力の一致を確認するためにコードが変化しないようにするため
に assert をコメントアウトする処理をテスト的に入れた。

a1d69d9 (#1179)

しかし、assert が独立した行に書かれていなかったため、一括置換するとビルドエラーに
なったため手動で除外する対応が必要になった。

一括置換で対応できるように (スクリプトで自動化できるために) 独立した行にする

PR のメリット

#1179, #1172 の対応を行うときに assert をテスト的に無効化する処理をスクリプトで自動化できる。

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

なし

PR の影響範囲

関連チケット

#1179
#1172

参考資料

@m-tmatma m-tmatma added this to the v2.4.0 milestone Mar 20, 2020
@m-tmatma m-tmatma added the refactoring リファクタリング 【ChangeLog除外】 label Mar 20, 2020
@AppVeyorBot
Copy link

Build sakura 1.0.2660 completed (commit 11aa99c399 by @m-tmatma)

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

問題ないと思います。

PR趣旨は了解、方法論も問題なさげに見えました。

手順の整合性確認するためにパターンgrepしてみた感じ、対処漏れもなさそう・・・。

すべて検索 "^[\t ]*[^\t\n\r ]+[\t ]*\bassert(?:_warning)?\b", 大文字と小文字を区別する, 正規表現, 変更したファイルを閉じない, 検索結果 2, ソリューション全体, ""
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(31):#ifdef assert
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(32):#undef assert
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(41):	#define assert(exp) \
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(44):			debug_output("!assert: %hs(%d): %hs\n", __FILE__, __LINE__, #exp); \
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(49):	#define assert_warning(exp) \
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(58):	#define assert(exp)
  C:\work\sakura-editor\sakura_core\debug\Debug2.h(59):	#define assert_warning(exp)
  C:\work\sakura-editor\sakura_core\types\CType.h(295):		/*assert(IsValid());*/
  C:\work\sakura-editor\sakura_core\types\CType_Java.cpp(457):	L"assert",	// Mar. 8, 2003 genta
  C:\work\sakura-editor\sakura_core\types\CType_Python.cpp(562):	L"assert",
  C:\work\sakura-editor\sakura_core\CKeyWordSetMgr.cpp(589):	// assert( m_nKeyWordSetNum < MAX_SETNUM );
  C:\work\sakura-editor\sakura_core\CKeyWordSetMgr.cpp(590):	// assert( 0 <= nSize );
  C:\work\sakura-editor\sakura_core\CKeyWordSetMgr.cpp(615):	// assert( 0 <= nIdx && nIdx < m_nKeyWordSetNum );
  一致する行: 13    一致するファイル: 5    検索ファイル総数: 716

@berryzplus
Copy link
Contributor

#1179 が rebase される感じになるんでしょうか・・・?

いまの状態のイメージ

PR番号 内容 説明
#1172 ソース差分対策 + フォーマット適用 ここから差分対策を取り除いたものがマージされる。
#1179 ソース差分対策のみ #1172 で何を変えるのか見えるようにするための比較用(スタイル変更だけでアセンブラレベルにすると「変更なし」だよ、というためのモノ。

一括フォーマットに関しては、暫定標準を提案して「いったんこれで。」をやる感じになると思っております。たぶん clang-format のオプション項目を列挙して、「これは○○だから、こうする」というのを表にする感じかな?と思います。

@m-tmatma
Copy link
Member Author

#1179 が rebase される感じになるんでしょうか・・・?

rebase するよりは新しいものを作ることにはなると思います。

@m-tmatma m-tmatma merged commit 5564265 into sakura-editor:master Mar 21, 2020
@berryzplus
Copy link
Contributor

新しく作る、で問題ないと思います。

「オプションまとめ」のイメージ(ダメな例)

オプション項目 既定値 vs2017 の規定値 設定する値 理由
BasedOnStyle - Visual Studio(?) Visual Studio なんとなく。
ColumnLimit - - 80 伝統的にアレだから。
UseTab - - Never タブ記号を使うとエディタ設定によって「俺のコード」の見え方が変わるので許せません。
IndentWidth - 4 8 Javaの標準がタブ幅=8なんでソコに合わせてみます。

ダメポイント:

  • 理由「なんとなく」は説明になってねーです。
  • 理由「アレだから」は説明になってねーです。
  • 理由が極端に主観的な場合、同調するのが難しいです。
  • C++以外の他言語の基準に合わせる、という主張だと妥当性判断がほぼ不可能です。

妥当な値を決めてくのは結構大変そうですが、資料はそれなりに転がってそうでした。
http://algo13.net/clang/clang-format-style-oputions.html

@m-tmatma m-tmatma deleted the feature/divide-assert branch March 21, 2020 10:45
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…sert

assert を独立した 1 行で記載する
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.

3 participants