メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる #421

berryzplus · 2018-09-08T17:01:38Z

#398 の対策 PR です。

高速化とRFCへの対応を済ませたバージョンができたのでとりあえず上げときます。
まだちゃんとテストができてない状態です ~~が、たぶんもう変わらないと思います。~~
テストまで完了したので WIP 外します。

実行速度とRFC5321準拠に気を遣った結果、いくつかの点で挙動を変えています。
どのように変更があったかは単体テストに記載しています。

m-tmatma · 2018-09-09T05:26:53Z

#418 で @beru さんに単体テスト対応いただきましたが、

この対応も単体テスト書くにはぴったりだと思うので、
単体テスト対応していただけますか?

berryzplus · 2018-09-09T08:12:47Z

単体テスト対応していただけますか?

しました。が、元ファイルが単独ではビルドできないファイルなので、
該当部分のコードだけコピペして検証する方法を採っています。

berryzplus · 2018-09-09T08:17:14Z

~~何故だ。取消線が付かない・・・。~~

m-tmatma · 2018-09-09T12:21:45Z

しました。が、元ファイルが単独ではビルドできないファイルなので、

該当部分を別ファイルに移動して単独でコンパイルできるようにして対応できないですか?

m-tmatma · 2018-09-09T12:22:14Z

↑ 訂正

berryzplus · 2018-09-09T12:42:56Z

おお、取消線が！　あざっす＞ @beru さん

該当部分を別ファイルに移動して単独でコンパイルできるようにして対応できないですか?

了解です。やってみます。

berryzplus · 2018-09-09T19:52:16Z

変更をpushできたっぽいです 😄
レビューお願いします。

sakura_core/util/string_ex.cpp

sakura_core/util/string_ex.h

beru

処理速度がかなり改善されているので問題ないと思います。

berryzplus · 2018-09-09T21:38:18Z

あざーっす。

berryzplus · 2018-09-09T21:40:04Z

いちおうappveyorのビルド待ち・・・
始業までに終わっていたらいいな・・・

m-tmatma · 2018-09-09T21:43:23Z

tests/unittests/test-is_mailaddress.cpp

+/* 現在位置がメールアドレスならば、NULL以外と、その長さを返す
+	@date 2016.04.27 記号類を許可
+*/
+BOOL IsMailAddress_20160427( const wchar_t* pszBuf, int nBufLen, int* pnAddressLenfth )


旧コードを単体テストコード内の別ファイルに移動できないですか?

技術的に不可能ではありませんが、
依存関係をだましだまし組み込んだものなのでバランスを崩したくない感じです。
何故別ファイルに分ける発想になったか教えていただけますか？

単純に読みやすさのためです。
でも難しければ、別 PR でもかまいません。

m-tmatma · 2018-09-09T21:49:11Z

tests/unittests/test-is_mailaddress.cpp

+// このテストファイルローカルのテストモード切替フラグ
+//   0 で新旧比較モード
+//   1 で新の検証モード
+#if 0


コメントわかりにくないですか?
#if 0 → #if 1 にすると新旧比較モードになって
#if 0 のままにすると新の検証モードになります。

#if MODE_ISMAILADDRESS とかして上記の説明ならわかるのですが。

今後の検討課題としたいです。

当初これを書いたときは #if 0 のテストはある程度時間が過ぎたら削除してしまうつもりでいたので、「現新比較と新規分の検証さえできればいいや」でテキトーに書いてました。旧関数取込部分で指摘されている通り「ファイルを分けて永久保存」のが自然だと思うので、ファイルを分ける作業のときに分岐条件と説明を書きなおそうと考えています。

m-tmatma · 2018-09-09T21:52:43Z

tests/unittests/test-is_mailaddress.cpp

+	TEST20180909(TRUE, szTest, ::wcslen(szTest), NULL);
+}
+
+// 動作変更あり。新実装では条件を厳しくして高速化している


動作変更あり というのは、変更前後で結果が異なることを意味していますか?
その場合は TEST20180909 を使わずに、結果が違うことを前提にしたチェックに
したほうがよくないですか?

↑ を適用して、古いバージョンも新しいバージョンも常に実行できるようにしたほうがいいと思います。実行時間的には大したことないからです。

了解。競合があるかもなので未確定ですがこんな感じを考えています。

MY_ASSERT_SAME(expected, szTarget, cchTarget, pchMatchedLen); // 結果変わらない MY_ASSERT_CHANGE(expected, szTarget, cchTarget, pchMatchedLen); // 結果変わる

ds14050 · 2018-09-10T15:05:45Z

sakura_core/util/string_ex.cpp

+	const ptrdiff_t MAX_DOMAIN = 63;
+
+	// 関数仕様
+	assert(pszAtmark + 1 < pszEnd); // @位置と終了位置は逆転してはならない


a@[EOF]

という２文字のテキストが引っかかります。リリース版では a@ 部分がリンクになりました。

ひー。結局当たりだったので対応しました。

beru · 2018-09-10T22:53:51Z

PRへのレビューでは無いですがこんなページを見つけました。
http://emailregex.com/

ds14050 · 2018-09-11T09:17:23Z

雑な日本語訳でいっとき話題になった記事ではないですか？「99.99%マッチする」と書いてしまったために、100%のメールアドレスにマッチするパターンなんて簡単に作れるぜ、という反応を釣ってしまったという。(雑談失礼)

99.99% のレベルは損益分岐点を割り込んでいる気がします。そのくせケータイメールの一部の逸脱したアドレスにはマッチせず、現実の利益を失います。

ds14050 · 2018-09-11T09:26:13Z

もうひとつ気がついた点です。RFCによる文字数上限を、処理量の上限を定めて打ち切るためではなく、RFCに準拠しているかどうかという判断にのみ使用しているように読めました(見落としでなければ)。

これは faster_ismailaddress.txt では、上限より１文字だけ長いメールアドレス(候補)をいったんメールアドレスとして受け入れられるようにしておき、あとで非メールアドレスとして弾いていました。

とはいえ PR 版の判定関数がボトルネックになるという事実は確認できませんでした。

berryzplus · 2018-09-11T10:56:56Z

別件扱いで検討しときたい感じの話題がいくつか出てるので、マージはissue立ててからにしようと思います。

ds14050 · 2018-09-11T12:20:02Z

assert に引っかかる件( #421 (review) )はただのコメントではなく修正要求にすべきでした。マージの前に対応をお願いします> @berryzplus さん

berryzplus · 2018-09-11T12:38:58Z

assert に引っかかる件( #421 (review) )はただのコメントではなく修正要求にすべきでした。マージの前に対応をお願いします> @berryzplus さん

いま作業中で「え？」と思ったトコです。
もちろん確認します。

想定では a@(終端) でも a@[EOF] でも IsMailAddress 関数は FALSE を返すはず。

berryzplus · 2018-09-11T12:46:03Z

a@(終端) が TRUE を返しました。
バグですね。

原因確認して対応します。
@m-tmatma さんからのファイルを分ける提案と、
スイッチを識別子に変えた方が分かりやすいの提案も一緒に対応してしまおうと思っています。

m-tmatma · 2018-09-11T13:07:53Z

tests/unittests/test-is_mailaddress.cpp

+
+// テスト対象関数があるcppファイルを埋め込みで取り込む
+// 他ファイルで同じファイルを取り込んではいけない
+#include "util/string_ex.cpp"


このインクルードの代わりに CMakeLists.txt に追加で対応できないですか?

util/string_ex.cpp をテストするための exe を作る、という対応であればそのほうがincludeより簡単です。

ちなみに include で対応しようとして苦戦中ｗ

util/string_ex.cpp をテストするための exe を作る、という対応であればそのほうがincludeより簡単です。

util/string_ex.cpp、というか debug/debug2.cpp に激しい依存関係があるのをすっかり忘れておりました。cppファイルを単独でcmakeプロジェクトに追加してビルドしようとすると未定義の独自型のせいでエラーになります。あと、windows.hを普通にインクルードするとstd::minが使えなくて、それもエラーになってしまいます。

そうした方がいいのは指摘の通りなのでいつかそうしたいです。

あと、windows.hを普通にインクルードするとstd::minが使えなくて、それもエラーになってしまいます。

これは

#define NOMINMAX #include <Windows.h>

こうすると回避できます。

プロジェクト設定でdefineしてしまう、という手も・・・
sakura_coreはこの方式です。
makeが吐くコマンドラインが長くなるのでできるだけ回避したいです。

ここは少しずつ改善していける部分だと思っています。
いまは一旦includeによる間接取り込みを推したい感じです。

定数値の誤りを修正コメントの誤りを修正

beru

LGTM

beru · 2018-09-14T03:29:56Z

AppVeyor がうまく動いてませんが多分問題無いと思うので Merge してしまいます。
もし何か問題が見つかった場合は別のPRで対処する事にしましょう。

beru · 2018-09-14T03:32:24Z

sakura_core/util/string_ex.cpp

+	constexpr ptrdiff_t MIN_TLD = 2;
+
+	// ドメインの最小文字数
+	constexpr ptrdiff_t MIN_DOMAIN = 3;


こっちは 3 のままですが、https://github.com/sakura-editor/sakura/pull/421/files#diff-df29b4c2306b128b12d04d40f62ad60bR1051 の記述と食い違いますね。

ccTLDの最小文字数とドメインの最小文字数は関数内定数にせずにファイル内定数にして、
https://github.com/sakura-editor/sakura/pull/421/files#diff-df29b4c2306b128b12d04d40f62ad60bR1052
のように 6 を即値で書かないで、式で作ると値を統一出来て良かったかもしれないですね。

@以降の判定をもう少し簡略化したいなーと思っています。
「文字列がFQDNかどうか？」を終端で抜けるとき1回だけ調べるようにして、
その部分をIsURLと共有化できないかな、と。

berryzplus · 2018-09-14T11:29:57Z

あざーっす!

累積残課題が結構あるんですが、これで遅延問題は解決できたと思ってます。

ds14050 · 2018-09-14T15:12:56Z

終わったことですが、疑問点はつぶしておきたい性分です。

dot-String という構文規則がオクテットのくだりの根拠になります。

その構文規則は読んでいましたが、Atom = 1*atext の解釈ができませんでした。しかし、Wikipedia(ja) による ローカル部に使用できる文字は以下のASCII文字である。まず、次のASCII文字をそのまま並べた形式（RFC 5321ではDot-string、RFC 5322ではdot-atomと呼ぶ）が使用できる。 という説明を読んでいたので、単一の(=連続しない)ドットで区切られた半角英数記号の連続であろうという予想をしていました。

dot-string は「ドット区切りの 3桁8進数」の定義ではなさそうです。quoted-string 以外のすべての local-part が dot-string なのだから、通常見かけるメールアドレスのローカル部が dot-string であるはずなのです。

ds14050 · 2018-09-14T15:44:43Z

a@vvvv[EOF] という６文字のテキストがメールアドレスであると判定されたのですが、間違ったものをダウンロードしていますか？

サクラエディタ   v2.3.2.791 32bit
(GitHash 9c1868a38d25ac8d952c732770f7066ee10ab861)
(GitURL https://github.com/sakura-editor/sakura.git)

      Share Ver: 172
      Compile Info: V1915 WPR WIN500/I501/C000/N500
      Last Modified: 2018/9/15 00:20:03

berryzplus · 2018-09-14T15:55:00Z

a@vvvv[EOF] という６文字のテキストがメールアドレスであると判定されたのですが、間違ったものをダウンロードしていますか？

コードのコメントとずれてますが、正しい判定結果だと思います。
大きな影響はないと思いますが、判定結果やや微妙なのでもう一度整理が必要だと思っています。
後日、別issueを立てて書いていただいたコメントで未回答のものも対策していきたいと考えています。

ds14050 · 2018-09-14T16:12:20Z

後ろが[EOF]である場合にだけ起こるバグ(判定ミス)だと思います。

もうひとつ、a [email protected] が全体としてメールアドレスであると判定されますが、以前はスペースで途切れていました。これは RFC 対応による違いなのでしょうか(と調べる前に質問します)。なお、ダブルクリックして Thunderbird が起動すると宛先は自動的に "a a"@example.com になっていました。

berryzplus · 2018-09-14T16:30:03Z

dot-string は「ドット区切りの 3桁8進数」の定義ではなさそうです。

そういわれると正直あやしいので、解釈の流れを説明してみます。

まず、メールアドレスのBNFがこれです。

   Mailbox        = Local-part "@" ( Domain / address-literal )

   Local-part     = Dot-string / Quoted-string
                  ; 大文字・小文字を区別しなくてもよい(MAY)

local-part は dot-string または qutoted-string で構成される・・・

   Dot-string     = Atom *("."  Atom)

dot-string は atom とそれに続く0個以上の( "." に続く atom )で構成される・・・

   Atom           = 1*atext

atom は 1 とそれに続く0個以上の atext で構成される・・・

atext についての説明は見つからず。ここからは想像・・・

C言語の世界ならば 8進数は 077 のように書くのが普通だと思います。

何故「1 始まりの任意のテキスト」と解釈したかというと、
これが SendMail (=SMTP) について書かれた文書だから、ということになります。

chmod 755 test.sh と書くときの 755 が 8進数であることはご存じだと思います。
unix系文化では 8進数の 3桁数字が結構使われているような気がします。

ここで「1 始まりの任意のテキスト」に何か特定の意味を当てはめようとしてみます。
ASCIIコード 100 ～ 177 (0x40～0x7F) だと考えるとしっくりくるんです。
アメリカ人は文字を表現するのに 7bit しか使わないので、
この範囲で文字の大半を表せてしまうのです・・・

一番肝心な部分が「想像だった」ってオチなんですけどね。

berryzplus · 2018-09-14T16:37:25Z

もうひとつ、a [email protected] が全体としてメールアドレスであると判定されますが、以前はスペースで途切れていました。これは RFC 対応による結果なのでしょうか(と調べる前に質問します)。

これはバグですね。次リリースまでには直すべきものと認識しました。

k-takata · 2018-09-15T04:24:47Z

Atom = 1*atext

1* の表記はRFC 5234で定義されており、1個以上のatextの連続を意味します。atextはなぜかRFC 5321では一切言及がありませんが、RFC 5322には

   atext           =   ALPHA / DIGIT /    ; Printable US-ASCII
                       "!" / "#" /        ;  characters not including
                       "$" / "%" /        ;  specials.  Used for atoms.
                       "&" / "'" /
                       "*" / "+" /
                       "-" / "/" /
                       "=" / "?" /
                       "^" / "_" /
                       "`" / "{" /
                       "|" / "}" /
                       "~"

と定義されています。「ドット区切りの 3桁8進数」ではありません。

berryzplus · 2018-09-15T08:13:29Z

「ドット区切りの 3桁8進数」ではありません。

ありがとうございます。
条件修正しておかしくなってるのを直したいと思います。

This reverts commit fb81a39555d486d801eeb3488f0f37d795a0d350.

m-tmatma · 2019-03-08T20:43:56Z

#792 で revert された。
reverted というタグをつけてみた。

…_mailaddress_check メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる

This reverts commit fb81a39555d486d801eeb3488f0f37d795a0d350.

berryzplus changed the title ~~[WIP] 特定のファイルで描画が遅くなる現象への対応~~ メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる Sep 9, 2018

berryzplus force-pushed the feature/speed_up_mailaddress_check branch from 2bd80f2 to 0bd7882 Compare September 9, 2018 19:48

beru reviewed Sep 9, 2018

View reviewed changes

sakura_core/util/string_ex.cpp Outdated Show resolved Hide resolved

beru reviewed Sep 9, 2018

View reviewed changes

sakura_core/util/string_ex.cpp Outdated Show resolved Hide resolved

beru reviewed Sep 9, 2018

View reviewed changes

sakura_core/util/string_ex.cpp Outdated Show resolved Hide resolved

beru reviewed Sep 9, 2018

View reviewed changes

sakura_core/util/string_ex.h Outdated Show resolved Hide resolved

beru previously approved these changes Sep 9, 2018

View reviewed changes

berryzplus dismissed beru’s stale review via af364f9 September 9, 2018 21:33

beru previously approved these changes Sep 9, 2018

View reviewed changes

m-tmatma reviewed Sep 9, 2018

View reviewed changes

ds14050 reviewed Sep 10, 2018

View reviewed changes

m-tmatma reviewed Sep 11, 2018

View reviewed changes

レビュー指摘対応

69b1096

定数値の誤りを修正コメントの誤りを修正

beru approved these changes Sep 14, 2018

View reviewed changes

beru merged commit 9c1868a into sakura-editor:master Sep 14, 2018

beru reviewed Sep 14, 2018

View reviewed changes

berryzplus deleted the feature/speed_up_mailaddress_check branch September 14, 2018 11:30

beru mentioned this pull request Sep 14, 2018

特定のファイルで描画が遅くなる。 #398

Closed

ds14050 added enhancement ■機能追加 🚅 speed up 🚀 高速化 labels Sep 18, 2018

berryzplus mentioned this pull request Sep 29, 2018

RFC5321解釈の誤りを訂正する #493

Closed

berryzplus mentioned this pull request Dec 29, 2018

リリースに関して sakura-editor/management-forum#52

Closed

m-tmatma mentioned this pull request Jan 20, 2019

メールアドレスを含むテキストを開いた場合に、メールアドレスの区切りを超えてメールアドレスと認識される。 #775

Open

berryzplus added a commit to berryzplus/sakura-editor that referenced this pull request Feb 23, 2019

Revert "squash commit of sakura-editor#421"

93e3a89

This reverts commit fb81a39555d486d801eeb3488f0f37d795a0d350.

berryzplus mentioned this pull request Feb 23, 2019

#398 に対する回避策 (特定のファイルで描画が遅くなる) #792

Merged

m-tmatma added the reverted label Mar 8, 2019

m-tmatma mentioned this pull request Mar 8, 2019

IsMailAddress に対する単体テストを復活する #798

Closed

m-tmatma mentioned this pull request Mar 21, 2019

PR 421 によって導入されたけど revert された IsMailAddress の単体テストを復活する #808

Merged

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#421 from berryzplus/feature/speed_up…

c5cd5d1

…_mailaddress_check メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Revert "squash commit of sakura-editor#421"

4e517d8

This reverts commit fb81a39555d486d801eeb3488f0f37d795a0d350.

berryzplus mentioned this pull request May 9, 2020

シングルクォートで囲まれたメールアドレスを認識できるようにする #1289

Closed

メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる #421

メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる #421

Conversation

berryzplus commented Sep 8, 2018 • edited by beru Loading

m-tmatma commented Sep 9, 2018

berryzplus commented Sep 9, 2018

berryzplus commented Sep 9, 2018 • edited by beru Loading

m-tmatma commented Sep 9, 2018 • edited Loading

m-tmatma commented Sep 9, 2018

berryzplus commented Sep 9, 2018

berryzplus commented Sep 9, 2018

beru left a comment

Choose a reason for hiding this comment

berryzplus commented Sep 9, 2018

berryzplus commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru commented Sep 10, 2018

ds14050 commented Sep 11, 2018 • edited Loading

ds14050 commented Sep 11, 2018 • edited Loading

berryzplus commented Sep 11, 2018

ds14050 commented Sep 11, 2018

berryzplus commented Sep 11, 2018

berryzplus commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

beru commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus commented Sep 14, 2018

ds14050 commented Sep 14, 2018 • edited Loading

ds14050 commented Sep 14, 2018

berryzplus commented Sep 14, 2018

ds14050 commented Sep 14, 2018 • edited Loading

berryzplus commented Sep 14, 2018

berryzplus commented Sep 14, 2018

k-takata commented Sep 15, 2018

berryzplus commented Sep 15, 2018

m-tmatma commented Mar 8, 2019

berryzplus commented Sep 8, 2018 •

edited by beru

Loading

berryzplus commented Sep 9, 2018 •

edited by beru

Loading

m-tmatma commented Sep 9, 2018 •

edited

Loading

ds14050 commented Sep 11, 2018 •

edited

Loading

ds14050 commented Sep 11, 2018 •

edited

Loading

ds14050 commented Sep 14, 2018 •

edited

Loading

ds14050 commented Sep 14, 2018 •

edited

Loading