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

convert 配下のファイルを UTF-8 (BOM付) に変換 #264

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

kobake
Copy link
Member

@kobake kobake commented Jul 18, 2018

該当フォルダ内の文字コードをすべて UTF-8 (BOM付) に変換しました。

cd sakura_core/convert
nkf --overwrite --oc=UTF-8-BOM *.cpp
nkf --overwrite --oc=UTF-8-BOM *.h

確認方法

WinMerge で変更前と変更後を比較すると、文字コード以外の変更が無いことが確認できます。

関連 Issues

ソースコードのUnicode化 #112

特記事項

これまでの経緯から、マルチバイトを含む文字列リテラルについてもファイルのエンコーディングを変更したところで悪影響が起こらないように感じています。

今回の変更ではマルチバイトを含む文字列リテラルを含むファイルも含めてファイルエンコーディングの変更を行っています。

主に動作影響について対応前後で挙動に変化がないことのご確認をいただけると助かります。
(こういうときに単体テストを組んでおくと楽だったなーと思いました)

@berryzplus
Copy link
Contributor

主に動作影響について対応前後で挙動に変化がないことのご確認をいただけると助かります。

この一文はできれば外してチェックしたいです。

今回のコード変換のまとまりは確認が非常に困難だと思っています。
もともとどういう動作をすべきなのかが不明な機能が多いので、
対応前後で差異がないこと、または差異があるのを確認することは不可能に近いと思います。

一旦は、文字コード(と改行)以外に一切の差異がないことをもって「変換」を済ませてしまうことが先決であるように思っていますがどうでしょうか?

@kobake
Copy link
Member Author

kobake commented Jul 18, 2018

全挙動を見る必要は無く、1か所でもリテラル系が絡む動作が問題ないことが確認できれば良いかな、くらいの気持ちで書いています。

たとえば「全角→半角」のような変換機能が「それっぽく」動いていること、など。

@kobake
Copy link
Member Author

kobake commented Jul 18, 2018

一旦は、文字コード(と改行)以外に一切の差異がないことをもって「変換」を済ませてしまうことが先決であるように思っていますがどうでしょうか?

個人的にはそれで構わないと思っています。
なんとなく慎重にやりたい人もいるのかな、と思ってこういう感じの PR になってマス

@kobake
Copy link
Member Author

kobake commented Jul 18, 2018

特に異論なければ一旦動作チェック飛ばしてファイル内容比較のみでレビュー進めてしまいましょうか。

ちなみに

たとえば「全角→半角」のような変換機能が「それっぽく」動いていること、など。

等は個人的に既にチェック済みで、動作問題ありませんでした。

@m-tmatma
Copy link
Member

これまでの経緯から、マルチバイトを含む文字列リテラルについてもファイルのエンコーディングを変更したところで悪影響が起こらないように感じています。

ここの部分は慎重にやりたいと思います。
コメントだけならいいのですが、
文字列リテラルを含む場合に、影響はないのなら
なぜ影響がないのか、論理的な裏付けを取っておきたいです。

@berryzplus
Copy link
Contributor

変換結果をチェックしたい、というところには同意なんですが、
うまく行く理由「VC++の仕様です」じゃダメなんですかね?

ワイド文字列リテラル( L"あ" )についてはUTF-16LEに、
文字列リテラル( "あ" )についてはビルド環境のロケールに従ったエンコード(ShiftJIS)に、
それぞれ変換して格納するのがVC++の仕様です。
逆にこれが、ja-JP にしないとappveyorがうまく行かない理由です。

String and Character literal

う~む、書いてないですねぇw

ソースファイルと実行ファイルの文字コードを個別に指定できるGCCと比べると、VC++は「勝手にうまいことやってくれる部分」が多いので、基本的なところを確認しようとすると結構めんどうです。

何かやり方考えますか・・・

@m-tmatma
Copy link
Member

何か実験するなり、なんらかのドキュメントを見つけるなり、方法はなんでもいいのですが、
検証可能な根拠があればいいです。

@k-takata
Copy link
Member

ソースファイルと実行ファイルの文字コードを個別に指定できるGCC

VCにも /source-charset:xxx, /execution-charset:xxx, /utf-8というものがあります。念のため。

@berryzplus
Copy link
Contributor

ひとつ方法を思いつきました。

VC++にはアセンブラリストをUnicodeで生成する機能があります。
VC++が解釈したプログラムをUnicodeテキスト形式で出力する機能です。
この機能を使えば、VC++がソースコードをどのように解釈しているか知ることができます。

    <ClCompile>
      <AssemblerOutput>All</AssemblerOutput>
      <UseUnicodeForAssemblerListing>true</UseUnicodeForAssemblerListing>
    </ClCompile>

現状でアセンブラリストを比較すると CDlgAbout.asm 以外に差分はでません。
(CDilgAbout.asmには、バージョン情報文字列のリテラルが貼り付くので必ず差分が出る)

やや微妙な方法ですが、他になければこれでいくしかないかも。

@berryzplus
Copy link
Contributor

VCにも /source-charset:xxx, /execution-charset:xxx, /utf-8というものがあります。念のため。

マジっすか!・・・ほんとだ。

https://docs.microsoft.com/en-us/cpp/build/reference/execution-charset-set-execution-character-set
https://msdn.microsoft.com/en-us/library/mt708818.aspx

@kobake
Copy link
Member Author

kobake commented Jul 24, 2018

#290 を適用するため rebase しました

@berryzplus
Copy link
Contributor

この変更だけテスト書いてチェックしたいな、と。

他の変更と違って文字コード変換そのものを実装した機能だから、という理由で。

@m-tmatma
Copy link
Member

ビルド失敗してますね。
なんかブランチが存在しないというエラーになっているみたい。

@kobake
Copy link
Member Author

kobake commented Jul 25, 2018

謎の1個目のCI失敗が続く(RE-BUILD COMMIT しても出る)ので、例外的に close/reopen で対処します

Build started
git clone -q --branch=convert-utf8 https://github.com/sakura-editor/sakura.git C:\projects\sakura
fatal: Remote branch convert-utf8 not found in upstream origin
Command exited with code 128

@kobake kobake closed this Jul 25, 2018
@kobake kobake reopened this Jul 25, 2018
@berryzplus
Copy link
Contributor

ログ見るとブランチがない、と言ってるような・・・

@berryzplus
Copy link
Contributor

同じエラーですか。

@kobake
Copy link
Member Author

kobake commented Jul 25, 2018

なんかよくわからんですね……。タイムスタンプ変えて push -f してみます

cd sakura_core/convert
nkf --overwrite --oc=UTF-8-BOM *.cpp
nkf --overwrite --oc=UTF-8-BOM *.h
@kobake
Copy link
Member Author

kobake commented Jul 25, 2018

現 master から rebase しなおして push -f してみました

@berryzplus
Copy link
Contributor

元気に動き出した感じですね・・・

この変更だけテスト書いてチェックしたいな、と。

これやると時間かかりそうなので、一旦「無し」でお願いします。
文字コード変換だけじゃなく、base64とかuuencodeとかも含まれていたので全部検証するのは大変そうです。

他コミット同様のチェックでよいと思います。

@m-tmatma
Copy link
Member

#112 (comment)

現 master から rebase しなおして push -f してみました

rebase されたので、
https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.456
と比較するのでは駄目で、

https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.490
と比較すればよい。(ビルド中)

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.

LGTMです。

build456とbuild490でasmの一致を確認しました。

@kobake kobake merged commit c11b203 into sakura-editor:master Jul 25, 2018
@m-tmatma
Copy link
Member

build456とbuild490でasmの一致を確認しました。

説明不足ですみません。

rebase されたので、
https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.456
と比較するのでは駄目で、

build 456 と比較しては駄目です。

build490 と 049750d に対するビルドである build487 と比較する必要があります。
https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.487

@m-tmatma
Copy link
Member

https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.490 (ブランチ元の master)
https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.487 (PR の最新)

を比較して、CDlgAbout.asm 以外一致しているのを確認しました。

@kobake kobake deleted the convert-utf8 branch July 26, 2018 03:12
@m-tmatma m-tmatma added this to the next release milestone Aug 19, 2018
@ds14050 ds14050 added the refactoring リファクタリング 【ChangeLog除外】 label Sep 18, 2018
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
convert 配下のファイルを UTF-8 (BOM付) に変換
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