XML宣言を用いた文字コード判別処理を強化する #1422
XML宣言を用いた文字コード判別処理を強化する #1422
Conversation
ghost
mentioned this pull request
|
✅ Build sakura 1.0.3131 completed (commit 6f4e9bbbe8 by @kazasaku) |
sakura_core/charset/CESI.cpp
Outdated
| // { "utf-7", 5, CODE_UTF7 }, | ||
| // { "csutf7", 6, CODE_UTF7 }, |
There was a problem hiding this comment.
既存の記述に合わせています。
すぐ上の"utf-7"もコメントになっていますが、encodingNameToCode配列を新規追加したpatchunicode:#726の時点でコメントになっています。
（当該コミットは d2311e8 ）
何か理由があってUTF-7を判定対象外にしているのではないかと思っています。
There was a problem hiding this comment.
何か理由があってUTF-7を判定対象外にしているのではないかと思っています。
UTF-7のエンコーディング仕様はかなり複雑なので、自称UTF-7を鵜呑みにすると文字化けします。
https://ja.wikipedia.org/wiki/UTF-7
windowsには utf-7 と utf16le を相互変換する機能がありますが、サクラエディタでは UTF-7 コーデックを自作しています。自称 UTF-7 を UTF-7 の自作クラスで扱った場合に文字化けが起こるのを嫌って除外してあるんじゃないかと想像していますが、それでもコメントアウトにする（＝なかったことにする）の対応でいいのかは疑問です。コードの意図としては、エンコードが指定されていないと見做す（≒UTF8と看做す）がしたいわけではなく、AUTODETECTしたいはずなので想定結果がブレちまいます・・・。
sakura_core/charset/CESI.cpp
Outdated
| return static_cast<ECodeType>(encodingNameToCode[k].nCode); | ||
| } | ||
| } | ||
| for(k = i; pBuf[k] != quoteChar && k < nSize - 1; ++k){} |
There was a problem hiding this comment.
本来はループの外でやるべき判定を外に出す変更のようですね、良いと思います。
std::find_first_of を使うと処理の意味が明確になる気がしますが単に好みの話かもしれません…。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
文脈からMatchEncodingに分けた方を指していると誤解していました。失礼しました。
終点の引用符を探す処理、ループで行うよりはその方が分かりやすいと思います。
変更を入れておきます。
There was a problem hiding this comment.
本来はループの外でやるべき判定を外に出す変更
その視点で再考するとAutoDetectByCodingにこの変更を使わないのは変ですね。
これも変えておこうと思います。
sakura_core/charset/CESI.cpp
Outdated
| @@ -1226,6 +1277,10 @@ ECodeType CESI::CheckKanjiCode(const char* pBuf, size_t nBufLen) noexcept | |||
| if( nret != CODE_NONE && GetStatus() != ESI_NODETECTED ){ | |||
| return nret; | |||
| } | |||
| if( GetMetaName() == CODE_AUTODETECT ){ | |||
| // 2016.04.05 MetaがAUTODETECTの場合は、encodingがないxml文書。XML仕様書の通りUTF-8にする | |||
There was a problem hiding this comment.
個人的には元パッチに日付が付いていたからと言って、PRにする際にそれを残す必要は無いと思います。
意図的に残したいという事であれば反対はしませんが…。
There was a problem hiding this comment.
おいらの気が確かなら、「XML仕様の通りUTF-8にする」は誤解を招く表現かもです。
コンテキスト的にはUTF-8で正しいと思うんですけどね。
There was a problem hiding this comment.
従前の動作からすると、「これまで通りUTF-8とみなす」のほうが正しいような。
…とりあえずコメントの更新を入れておきます。
| @@ -501,11 +501,11 @@ void CESI::GetEncodingInfo_meta( const char* pS, const int nLen ) | |||
| { | |||
| // XML宣言は先頭にあるので、最初にチェック | |||
| ECodeType encoding = AutoDetectByXML( pS, nLen ); | |||
| if( encoding == CODE_NONE ){ | |||
| if( encoding == CODE_NONE || encoding == CODE_AUTODETECT ){ | |||
There was a problem hiding this comment.
この変更の妥当性にちょっと疑問があります。
こういうことなんじゃないかと。
// XML/HTML は自動判別と仮定する
bool altered = false;
if( encoding == CODE_NONE ){
encoding = CODE_AUTODETECT;
altered = true;
}
// 従来の判定文をちょっと変える
if( encoding == CODE_AUTODETECT ){
....(省略)
}
// 判別後に状態が変わって無かったら NONE に戻す
if (altered && encoding == CODE_AUTODETECT ){
encoding = CODE_NONE;
}で、その変更意味あるの？と言われると、ないかも知れない・・・なんですが。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
この指摘は一旦スルーして進めて構わんです。
たぶん、パッチの作成方針に根本的な問題があるっす。
CESI::GetEncodingInfo_meta というメンバー関数は m_eMetaName( == GetMetaName() ) を初期化するんですけど、この関数の結果を使っていいのは、BOMなしのマルチバイト文字コードのときだけな気がします。
| 文字コード | 先頭数bytes | 備考 |
|---|---|---|
| UTF-8(UTF-8BOM) | "\xef\xbb\xbf" | xmlかどうかに関わらず確定してOKなはず。 |
| UTF-16(UTF-16LE+BOM) | "\xfe\xff" | xmlかどうかに関わらず確定してOKなはず。 |
| UTF-16(UTF-16BE+BOM) | "\xff\xfe" | xmlかどうかに関わらず確定してOKなはず。 |
| UTF-16(UTF-16BE) | "\0<\0?\x00x\0m\0l" | encoding指定に関わらず確定してOKなはず。 |
| UTF-16LE | "<\0?\x00x\0m\0l\0" | encoding指定に関わらず確定してOKなはず。 |
| encoding依存 | "<?xml" | encoding指定が認識できる場合、それに従う。 |
| タイプ別設定依存 | "<?xml" | デフォルトエンコーディングに従う ※1 |
※1 encoding指定がない、または、認識できない場合、かつ、タイプ別設定でデフォルトエンコーディングが指定されている場合は指定されたデフォルトエンコーディングで確定する。デフォルトエンコーディングがない場合、UTF-8とする。
CESIクラスはタイプ別設定のデフォルトエンコーディング情報を持っています。
判定条件を精査すると AUTODETECT を返すべきパスはないことが分かります。
でもま、旧パッチ適用していまある問題を解決しようぜ！ がこのPRの趣旨だと思うのでこのままマージに向かってもよいと思います。
{ "windows-1252", 12, CODE_LATIN1 },
...
{"windows-1252", 12, 1252},latin1 (ISO-8859-1) と windows-1252 (CP1252) は本来別で、windows-1252 は latin1 のスーパーセットなのですが、サクラエディタではどちらも windows-1252 として扱ってるんですかね？ |
ISO-8859-1とWindows-1252では、0x80から0x9fの範囲が異なっていますが、それ以外は共通だったと思います。 ただ、CODE_LATIN1のときに使用されるCLatin1::Latin1ToUni関数は、0x80～0x9fの場合にMultiByteToWideChar関数を呼び出しており、第1引数にコードページ「1252」を指定しています。 |
|
✅ Build sakura 1.0.3136 completed (commit a6cd22f812 by @kazasaku) |
|
✅ Build sakura 1.0.3137 completed (commit 6def41abc4 by @kazasaku) |
欧米の文字コードには 7bit 版の文字位置を規定する iso 646 というのがありまして、 サクラエディタが iso-8859-1 を windows-1252 にマップしている理由を想像できる資料として以下のものがあります。
サクラエディタは「多くのWebブラウザや電子メールクライアント」に準ずる存在だと思うので、ISO-8859-1でも |
別途issueで検討したほうがよさそうですね。 |
latin1 は cp1252 として扱われているということが明確になりましたし、含めても問題ないと思います。
コミットは分かれていますし、それで十分ではないでしょうか。 |
|
#1422 (comment) |
https://sourceforge.net/p/sakura-editor/patchunicode/1050/ skrw_fix_xml_detect_v0_5.patch ・いくつかのエンコーディング名の別名を追加 MS932やSJIS、ハイフン・アンダーバー考慮 ・xml encoding名が不明な名前だった場合にUTF-8と認識されるバグを修正 例 <?xml version="1.0" encoding="MS932" ?> ・xml宣言がありencoding名がない場合は、UTF-8固定だった(規格どおり)ものを自動認識を優先してそれでも不明ならUTF-8にするように修正
Windows-1252に対してCODE_LATIN1と1252の二つが定義されており、 それぞれの変換処理には前者の場合CLatin1::Latin1ToUni関数、後者の場合CCodePage::CPToUni関数が使われている。 Latin1(ISO-8859-1)とWindows-1252で差異のある0x80~0x9fの変換には、 どちらにおいてもMultiByteToWideChar関数にコードページ1252を指定して実行するようになっているため、 Latin1とWindows-1252を区別せずに処理しているように見えることから、後者の指定を除去することとした。
|
✅ Build sakura 1.0.3144 completed (commit 7e54c8f69b by @kazasaku) |
| @@ -1226,6 +1273,10 @@ ECodeType CESI::CheckKanjiCode(const char* pBuf, size_t nBufLen) noexcept | |||
| if( nret != CODE_NONE && GetStatus() != ESI_NODETECTED ){ | |||
| return nret; | |||
| } | |||
| if( GetMetaName() == CODE_AUTODETECT ){ | |||
| // MetaがAUTODETECTの場合は、encodingがないxml文書。これまで通りUTF-8とみなす。 | |||
| return CODE_UTF8; | |||
There was a problem hiding this comment.
おそらく最後の確認事項です。
ここにこの分岐を入れると、encoding指定がない、または、encoding指定を認識できない場合にデフォルトエンコーディングを使う、という仕様は実現できなくなるっす。XML規格的にはこちらが正しいですがそれでいいです？
There was a problem hiding this comment.
#1422 (comment)
「一旦これで。」の確認がとれれば、動作確認して終わりな認識です。
There was a problem hiding this comment.
このまま進めてもよいのであれば、これでお願いします。
タイプ別設定を使うようにするPRは後から出します。
一応、同時対処してタイプ別設定を使う動作にしたものも残しておきます
→ master...kazasaku:feature/fix_detect_xml_charset
|
@berryzplus さん 自分はsf.netに残された不具合修正を目的としたパッチのうち、手元で再現とその修正が確認できたものを可能な限りそのまま取り込むことを目的に作業していました。 その一方で、encoding指定がない・あるものの不明なエンコーディング名だった時にこれまで（おそらく仕様に基づいて）UTF-8とみなしていた動作を、自動認識の結果を優先するものの最終手段としてUTF-8とみなすように変更しており、すべて失敗だった時の動作だけは変えていないように見えます。 仮にタイプ別設定も使うようにするならば：
|
This comment has been minimized.
This comment has been minimized.
sakura_core/charset/CESI.cpp
Outdated
| } | ||
| } | ||
| std::string sBuf = pBuf; | ||
| return MatchEncoding( pBuf + i, sBuf.find_first_of( quoteChar, i ) - i ); |
There was a problem hiding this comment.
このまま進めるみたいなので、スマホから失礼します。
ケチ臭い指摘で大変恐縮ですが、string_viewのほうが例外安全でスケールもしやすいと思います。
string_viewはC++17からでVC2017も対応済みです。
sBufのコンストラクタ引数のpBufは見た感じNUL終端文字列ではないかもしれません。長さも指定するのがおすすめです。
std::string::find_first_ofはstd::find_first_ofと違って、見つからないときに、npos(-1)を返します。npos-iが少し健康的ではない気がします。
(実際の動作上は問題ないですけど、MatchEncodingの長さの引数がマイナス値になります)
There was a problem hiding this comment.
ありがとうございます。
確かにNULL終端がないように見えますね。呼び出し元から渡された長さ情報（nSize）を使って、std::string_viewで構築するようにしておきます。
また、MatchEncodingの第2引数が負数だと判定は成功しないはずなので、これを呼ぶ必要はたぶんないです。
encoding指定が引用符で括られていないときは判定を省略しているようなので、find_first_ofでnposが返ってきたら指定が閉じられていないものと判断し、同様に取り扱うようにします。
There was a problem hiding this comment.
NUL終端は、付けたらいいと思います。
文字列として扱うバイト配列をNUL終端せずに使うことが不自然です。
この辺で確保量を +1 してやればよいはず。
sakura/sakura_core/charset/CCodeMediator.cpp
Lines 69 to 70 in 455a7ff
呼出元は他にもあるので、対応するなら網羅する必要ありです。
ここの関数はC++で普通に書いたほうが（≒正規表現で書いたほうが）分かりやすいと思います。
もちろん書換は別件で構わないです。
::find_first_ofの戻り値がnposだった場合、MatchEncodingの第2引数が負数となってしまうため、 MatchEncodingでの判定処理が成功しなくなってしまう。 もしnposの場合は判定を省略し、指定がなかったものとして扱うようにする。 std::stringを使用していた箇所をstd::string_viewを使用するように変更する。
|
✅ Build sakura 1.0.3147 completed (commit 18f1266276 by @kazasaku) |
There was a problem hiding this comment.
問題ないと思います。
| ファイル名 | 455a7ff | 0ea3203 | 解説 |
|---|---|---|---|
| テストファイル_01.xml | UTF-8 | SJIS | encoding が認識されるようになった |
| テストファイル_02.txt | EUC | EUC | coding が認識されるようになった |
| テストファイル_03.html | JIS | JIS | content="charset=xxx" が認識されるようになった |
| テストファイル_04.html | JIS | JIS | charset="xxx" が認識されるようになった |
| テストファイル_05.xml | UTF-8 | SJIS | 認識できないencodingが無視されて自動判別された |
| テストファイル_06.xml | UTF-8 | UTF-8 | XML宣言のencoding省略によりUTF-8と判定された |
|
レビューありがとうございました。 |
|
beru様、ありがとうございます。 |
ghost
deleted the
|
スルーしてしまったテストの問題点を一応共有しときます。
それぞれ全角文字を削って 7bit ASCII で表現できる文字のみにした状態で、変更前が SJIS と判定されることと変更後に指定された文字コードで認識されることを確認しときました。 |
続きの作業をしていてASCIIだけならCodingやHTMLも確認できることに（今更）気が付きました。 |
ghost
mentioned this pull request
PR の目的
XML宣言のencoding指定を使った文字コード判別処理を強化し、文字化けが発生する可能性を減らします。
カテゴリ
PR の背景
XML宣言のencoding指定に未知のエンコーディング名が指定されていたとき、これをUTF-8として認識するバグがありました。
patchunicode#1050で提案されている修正パッチを取り込み、この問題を解決するものです。
PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
このPRに含まれる変更は次の通りです。
XML仕様に基づきこれまで通りUTF-8として扱う。ENCODING_NAMEを定義して文字数を自動算出するように変更し、文字数の数え間違いによってエンコーディング名が認識されなくなる問題を防ぎます。テスト内容
テスト1（追加したエイリアスを認識するか）
テスト2（未知のエンコーディング名のとき）
テスト3（encoding指定がないとき）
テストファイルの説明
テストファイル.zip
sjis）をXML宣言のencodingに指定cseucpkdfmtjapanese）を1行目の# codingに指定csiso2022jp）をcontent属性のcharsetに指定iso2022jp）をcharset属性に指定shifutojisu）を指定PR の影響範囲
関連 issue, PR
BugReport/195
patchunicode#1050
#1418
参考資料