[x64対応] バージョン情報にPlatform情報を埋め込み (32bit/64bit) #90
Conversation
kobake
commented
Jun 11, 2018
•
kobake
commented
- exe, dll のバージョン情報にPlatform情報を埋め込み (32bit/64bit)
- バージョンダイアログ表示にPlatform情報を埋め込み (32bit/64bit)
kobake
changed the title
|
整備完了しました。レビューお願いします。 |
kobake
changed the title
あ、直します |
|
文字コードの変更が入ってますが |
順次、UTF-8 に動作影響のないソースコードは UTF-8 にしていきたいと思っています。 |
|
x64 の対応はそこそこ 時間がかかると思うので、 |
うーん、文字コード変更程度であればコンフリクト解消は難しくないと思っていますがどうでしょうか。適当なタイミングで随時 x64 に対して master をマージしていくことによって足並みは合わせられると思います。 むしろ文字コード変更しないことによって GitHub 上でのレビューが正確にできない（文字化けがあって見にくいところがある）ことのほうが自分は気にしています。 ※ .rc ファイルに限っては怖いので文字コード変更は控えています。 |
|
補足しておくと、今回文字コード変更したファイルはバージョン情報関連だったので文字列影響を精査したい気持ちが強く、文字コード変更の影響チェックと文字コード変更の実施を行いました。 このような特段の理由がない限りは文字コード変更はしないスタンスで行こうと思いますが、実際に作業しながら他にも気になったファイルがあれば文字コード変更は一応視野には入れておきたいです。 |
|
後々に検証が容易になるように、 面倒ですが。 |
|
コンフリクト以外でどのように困ることがありそうか具体例出していただけると理解しやすいです |
|
変更内容をレビューに時間がかかることです。 必要な修正以外に文字コード変更があると、 |
おっしゃることは分かります。 ひとつ論点を整理したいのですが、
これはこの話に関連ありますか？ |
PR を作ってすぐにマージされるものであればコンフリクトする可能性が低いので問題ないが、
コミット毎にチェックする場合は有効なのですが、 |
この PR を master に投げて、master 側に取り込まれた後に x64 に対して master のマージを走らせるとしたらどうでしょうか。 |
反対する主要な理由は上記です。 もし文字コードを変更されたいのであれば、文字コードを変更する PR を master を その場合でも x64 対応状況をまとめるチケットや wiki を整備されたように
この PR は x64 ブランチで対応した x64 用の構成の追加が前提となっているので |
master に適用した場合は単にバージョン情報に |
PR の限定方針については概ね同意です。 ただ、ファイルの文字コード変更については例外として扱いたいと考えていて、 諸々考えましたが、全ファイルを UTF-8 に変更するだけのために労力を割くのは現実的でないというのが今の時点での所感でして、ファイルの文字コード変更については何かしらの変更のついでにやっちゃうくらいの感じでじわじわと UTF-8 に変換していくのが現実的かと考えています。
ここまでやる必要は無いかな、というのが自分の考えです。 UTF-8 の対応状況を確認したいときにはチェックコマンド走らせるだけで分かるので、それで十分かと思っています。 |
master では、64bit の構成の対応が入っていなので 64bit 部分のテストができない
これに反対です。
これは賛成です。 |
文字コード変更だけの修正を x64 ブランチ宛ての PR として作ることについてはどう思われますか。 |
文字化けしてますか? ↓ これは別の PR ですが、特に文字化けしてません。 どんな環境で見てますか? |
GitHub 上での表示は SJIS 判定がうまくいく場合とうまくいかない場合があります（ファイルに含まれている文字群の統計から判別していると思われるので誤判定はしばしばあります）。 以下等は文字化けしてます。 あと自分のローカル作業環境では Git Bash を使っている都合上ぜんぶ文字化けします。 |
どのファイルですか?
これに関しては対策法はわかりませんが |
リソースファイルです。リソースファイルは SJIS のままにしてあるので文字化けしています。 |
対象ファイルが SJIS である場合と UTF-8 である場合があるので、頑張って設定するとしたら iconv とかを挟むと自動判別できるかもしれませんね……。誤判定は常に起こり得ますが。 |
|
話繋がりました。 色々端折りますが、utf-8に変更する理由に合点が付きました。dlgのところでも書きましたがすべてのsjis文字はutf-8に変換出来ますので、全ファイル一括変換で良いと思います。 懸念はMinGW向けカスタムツールへの影響ですかね。 |
|
Sjis->utf8をやって問題が出る可能性。うまく変換出来ないケースがあるのを思い出したので、紹介します。 鉄道会社は、社名を電子文書に起こす時に金を失わないための工夫をするそうです。 今でこそ、UNICODEには異体字というものがあります。鉄道会社は異体字を使って金を失うことを回避します。厳密には異体字じゃない点はスルーしてください。 昔はコードが割り当てられていない領域を活用する外字で対応していたらしいです。 本来sjisに割り当てられていない文字というのは、標準環境では化ける文字です。こういうのは流石に変換出来ません。サクラエディタのソースコードは標準環境では文字化けしないと思っています。 |
kobake
changed the title
金？"金"？money？ |
| HIWORD( dwVersionLS ), | ||
| LOWORD( dwVersionLS ) | ||
| auto_sprintf(szMsg, | ||
| _T( |
There was a problem hiding this comment.
auto_sprintf では _T を使っていますが、
VER_GITHASH では _T を使っていません。
_T を使えば hs を使わなくてもいいのでは?
読みにくいからですか?
There was a problem hiding this comment.
_T("aaaa") みたいに一目で中身が文字列リテラルだと分かるものには _T 使うこと多いですが
_T(HOGE_HOGE) みたいなものだと HOGE_HOGE がそこだけ見ると文字列リテラルかどうか判断付かない（定義元まで見ないと分からない）ので、そういうときは _T は使わないことが多いです。
There was a problem hiding this comment.
_T("aaaa") みたいに一目で中身が文字列リテラルだと分かるものには _T 使うこと多いですが
文字列リテラルで _T を使う例は確かに多いです。
ただ、GitHub の他のコードを見ると _T 込みでマクロ定義してものもありますし、
また Windows 標準のヘッダの中で _T 込で定義しているマクロもあります。
_T ではないですが、同様の役割の TEXT マクロで
TEXT マクロ込みで定義されているマクロ名はいっぱいあります。
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17134.0 以下だと 2800 個くらい。
_T(HOGE_HOGE) みたいなものだと HOGE_HOGE がそこだけ見ると文字列リテラルかどうか判断付かない
_T の意味を知っていたら、わかると思います。
176 行目で _T 使っていますし。
There was a problem hiding this comment.
_T の意味はこれまで散々使ってきているので把握しています。
今回のケースで _T を使うとしたら gitrev.h のほうに入れるのが個人的な感覚です。
There was a problem hiding this comment.
今回のケースで _T を使うとしたら gitrev.h のほうに入れるのが個人的な感覚です。
はい。そのほうがきれいです。
There was a problem hiding this comment.
hsを避けたい理由があるのであれば先に説明していただければもう少しスムーズに話が進んだと思います……
There was a problem hiding this comment.
https://msdn.microsoft.com/ja-jp/library/tcxf1dw6.aspx
hs は Microsoft の拡張なので使わなくて済むのなら使わないほうがいいと思います。
There was a problem hiding this comment.
そうなんですね。そのリンクだけだとMS独自のものかどうか判断できませんでしたが。
とりあえず致命的な問題ではないので本PRについてはそのままとさせてください。
There was a problem hiding this comment.
メモ のところに書いてあります。
h プレフィックス (char 型のデータと共に使用する場合)、w プレフィックス (wchar_t 型のデータと共に使用する場合)、および l プレフィックス (double 型のデータと共に使用する場合) は Microsoft 拡張機能です。
There was a problem hiding this comment.
なるほど、スマホから見ていたもので見逃していました。
sakura_core/version.h
Outdated
|
|
||
| // リソース埋め込み用バージョン文字列 | ||
| // e.g. "2.3.2.0 (4a0de579) UNICODE 64bit DEBUG" | ||
| #define RESOURCE_VERSION_STRING(VERSION_STRING) VERSION_STRING " (" GIT_SHORT_COMMIT_HASH ") " VER_CHARSET " " VER_PLATFORM " " VER_CONFIG |
There was a problem hiding this comment.
VERSION_STRING というマクロ引数が大文字ですが、
大文字だと定数と見分けがつきにくいので通常の変数の
ように大文字、小文字を混ぜるか、全部小文字にするか
したほうがいいと思います。
There was a problem hiding this comment.
たしかに WinSDK のマクロも引数は小文字ですね。直します。
There was a problem hiding this comment.
小文字、というか以下2パターンありました
- 小文字
- 先頭アンダースコアの PascalCase
| @@ -0,0 +1,25 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
#pragma once が MinGW でサポートされているのか気になったので
調べてみた。→ サポートされているみたい。
https://stackoverflow.com/questions/28772519/does-mingw-4-8-2-support-pragma-once
kobake
changed the title
|
rebaseしてWIP外しました。内容はエンコーディング以外は変わってないです。 ひとつ念頭に入れていただきたい点としては、SJIS で変更を加えたコミットを後から UTF-8 変更されたものに対して rebase をかけようとすると、コンフリクト解消の手間がかなり大きくなるという点です。 そのような事情があるため、初期 PR では先に UTF-8 変換をかけた後のコード変更にしていたのですが、諸々議論が平行線をたどりそうだったので今回は自分が妥協して SJIS のままコード変更を行い、UTF-8 とのコンフリクト解消については（個人的な所感としては）コストに見合わないコンフリクト解消作業が発生していた事実はご報告しておきあす。 これはご自身で実際に試してみるとよく実感できるところかと思います。 |
はい。やる前から指摘していました。 でも、文字コード変換しないと文字化けして大変だから、コンフリクトの解決は問題ないから 別の PR でやってもいいから、やりたいとおっしゃってやった経緯があります。 文字コード変換をこの PR がマージした後にやれば今回のコンフリクト解決の |
訂正 |
いえ、当初の UTF-8 変更コミットを先に積む形であればコンフリクト解消はかなり楽でした。 例えば 45327cd 等は UTF-8 変更コミットを先に積まない場合は泣く泣く SJIS コメント周辺のコード変更を加える形になってしまっていましたが、それだと UTF-8 変更コードとのコンフリクト解消をする際に SJIS, UTF-8 混在のコンフリクトコードができあがってしまうので、コメント文は文字化けしてコンフリクト解消の手間がかなり大きく辛かったです。これはツールでなんとかなる問題ではありません。実際に試してみるとよく分かると思います。 当初の UTF-8 変更コミットを先に積んだ状態からのコミットであれば UTF-8 コメント周辺のコード変更になるため UTF-8 同士のコンフリクトなので解消は圧倒的に楽でした。 |
作業順序を考慮する必要があるのであればブランチを切る意味がないです。 |
|
|
||
| // リソース埋め込み用バージョン文字列 | ||
| // e.g. "2.3.2.0 (4a0de579) UNICODE 64bit DEBUG" | ||
| #define RESOURCE_VERSION_STRING(_VersionString) _VersionString " (" GIT_SHORT_COMMIT_HASH ") " VER_CHARSET " " VER_PLATFORM " " VER_CONFIG |
There was a problem hiding this comment.
Release 構成のときに、
VER_CONFIG の前に不要な空白が入ります。
些細なことですが。
VER_CHARSET ～ VER_CONFIG の定義で空白付きのマクロを定義して、RESOURCE_VERSION_STRING で使えば定義が見やすく定義しやすくなります。
There was a problem hiding this comment.
末尾空白入ることがあるのは把握していたのですがコードの見易さ優先してこうしてしまいました。
もうちょっと良い例があればご提示いただけると助かります。
There was a problem hiding this comment.
例えばこういうのを用意する、って意味でしょうか。
#ifdef _DEBUG
#define SPACE_WHEN_DEBUG " "
#else
#define SPACE_WHEN_DEBUG ""
#endif
There was a problem hiding this comment.
VER_CHARSET, VER_PLATFORM, VER_CONFIG については他の箇所でも流用しているので、この定数内にスペースを含めるのは見通しが悪くなるので避けている事情があります。
There was a problem hiding this comment.
こんな感じ
#ifdef _UNICODE
#define VER_CHARSET "UNICODE"
#else
#define VER_CHARSET "ANSI"
#endif
#define VER_CHARSET_WITH_SPACE " " VER_CHARSET
#ifdef _WIN64
#define VER_PLATFORM "64bit"
#else
#define VER_PLATFORM "32bit"
#endif
#define VER_PLATFORM_WITH_SPACE " " VER_PLATFORM
#ifdef _DEBUG
#define VER_CONFIG "DEBUG"
#define VER_CONFIG_WITH_SPACE " " VER_CONFIG
#else
#define VER_CONFIG ""
#define VER_CONFIG_WITH_SPACE ""
#endif
There was a problem hiding this comment.
ご提示ありがとうございます。
所感としては、 _WITH_SPACE というプリフィックスが付いている定数にスペースが入ってないことがあるというのは直観的にちょっと気持ち悪いなーという気持ちがあります。
自分が真面目に書くとしたら以下のようになりますね。
#ifdef _UNICODE
#define VER_CHARSET "UNICODE"
#else
#define VER_CHARSET "ANSI"
#endif
#ifdef _WIN64
#define VER_PLATFORM "64bit"
#else
#define VER_PLATFORM "32bit"
#endif
#ifdef _DEBUG
#define VER_CONFIG "DEBUG"
#else
#define VER_CONFIG ""
#endif
#ifdef _DEBUG
#define SPACE_WHEN_DEBUG " "
#else
#define SPACE_WHEN_DEBUG ""
#endif
// リソース埋め込み用バージョン文字列
// e.g. "2.3.2.0 (4a0de579) UNICODE 64bit DEBUG"
#define RESOURCE_VERSION_STRING(_VersionString) _VersionString " (" GIT_SHORT_COMMIT_HASH ") " VER_CHARSET " " VER_PLATFORM SPACE_WHEN_DEBUG VER_CONFIG
ここまでやったほうが良いですかね。
There was a problem hiding this comment.
それでもいいです。
RESOURCE_VERSION_STRING の定義で、マクロと文字列定数が混ざっていて見にくいので
改行したほうがいいと思います。
#define RESOURCE_VERSION_STRING(_VersionString) \
_VersionString \
" (" \
GIT_SHORT_COMMIT_HASH \
") " \
VER_CHARSET \
" " \
VER_PLATFORM \
SPACE_WHEN_DEBUG \
VER_CONFIG
There was a problem hiding this comment.
まだ出かけてなかったので再コメント
個人的には文字列自体に改行コード含まれているのでなければ1行で書きたい派です。
こんな感じ。
#define RESOURCE_VERSION_STRING(_VersionString) \
_VersionString " (" GIT_SHORT_COMMIT_HASH ") " VER_CHARSET " " VER_PLATFORM SPACE_WHEN_DEBUG VER_CONFIG
x64 ブランチに対して、UTF-8 を全体的に導入するという方法のことですか?
ブランチ作成の目的はブランチ元に影響しない用にすることで作業順序 また作業順序を考慮するのは不要な作業をなくすめための生活の知恵ですし
文字コード変換をしたいのであれば x64 ブランチを作る前あるいはマージ後に
これは大変なのは想像しているのでは私はこの作業自体を避ける方向で
実際に作業を開始して、rebase の手間が多いと判断した時点でこの PR を rebase で 作業のやり直しが大きい修正の場合はそもそも多数コンフリクトが発生するような修正を プログラムの構造を大きく変えるような修正を行ってコンフリクトが発生するような |
NOです。SJISコメント付近を修正するときのみ、あらかじめUTF-8に変換してから作業したいという感じです。これも生活の知恵です。
タイミングについては言及していません。 この PR の当初の姿のように、随時必要に応じて UTF-8 変換コミットを混ぜるのが個人的には手間が少なく楽と判断しました。 |
|
諸事情でリポジトリの Fork をし直したため、この PR の書き換えは不可。クローズして #179 で続きをやります。 |
