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

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

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Sep 29, 2018

目的

#421 の改善です。

取込済みPRでRFC5321を誤って解釈していたのを訂正します。

誤解釈によって、正しくないメールアドレスをメールアドレスと判定しています。
GitHub移行後に組み込んだバグなので対処は必須と考えています。

経緯

@berryzplus

#398 の対策 PR です。

@ds14050 さん #421 (comment)

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

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 さん #421 (comment)

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

@berryzplus #421 (comment)

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

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

@ds14050 さん #421 (comment)

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

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

@berryzplus #421 (comment)

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 #421 (comment)

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

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

@k-takata さん #421 (comment)

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 #421 (comment)

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

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

@berryzplus berryzplus added 🐛bug🦋 ■バグ修正(Something isn't working) IMPORTANT 早急に解消すべきもの labels Sep 29, 2018
/*
* RFC5321 let-dig の要件を満たすかどうか判定する
*
* 高速化目的で可読性を捨て、あえて冗長な switch 分岐にしている。
Copy link
Member

Choose a reason for hiding this comment

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

可読性の高いバージョンも用意して結果が一致することを確認する単体テストを用意する方がいいです

Copy link
Contributor Author

Choose a reason for hiding this comment

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

static関数なので単体テストに入れるのは難しいです。
関数自体をコピペで入れないといけないので。

いちおうテストは書いて、ローカルで実行してみました。

TEST(testIsMailAddress, CheckLetDig)
{
	constexpr wchar_t letDigChars[] = L"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
	for (wchar_t ch = 0; ch < 0xFFFF; ch++) {
		if (ch == 0) continue; //可読性高いverはNUL文字に対応しないので除外する
		bool resultOld = (::wcschr(letDigChars, ch) != nullptr);
		bool resultNew = IsLetDig(ch);
		ASSERT_EQ(resultOld, resultNew);
	}
}

Copy link
Member

Choose a reason for hiding this comment

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

static関数なので単体テストに入れるのは難しいです。

それなら、static を使わないようにすればいいと思います。
内部利用用の namespace を定義してその中に入れたらいいと思います。

明示的に内部用の namespace を using するか、namespace 指定しない限りアクセスできないです。

例えば googletest は internal という namespace を使っています。

https://github.com/google/googletest/blob/440527a61e1c91188195f7de212c63c77e8f0a45/googlemock/include/gmock/internal/gmock-internal-utils.h#L49-L50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

他関数からの利用を考えなくても良いようにstaticにしてます。

namespaceを付けて公開ヘッダーに移動すると、他関数に流用される危険が出ます。
他関数から利用されてもよいように再設計する必要が出てきます。

現状、コントリビューターに無理な負担を強いない、が基本だと思っています。

基本原則を曲げてまでこの関数のテストを書きたい理由が分からないです。
IsLedDigは今回追加した中ではかなり単純な部類なので意図を図りかねています。

Copy link
Contributor

Choose a reason for hiding this comment

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

static 関数をテストするために .cpp ファイルをインクルードすることがあるそうです。テストファイルから #include "util/string_ex.cpp" すれば実質的にはコピペですがコピペではありません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Includeはテスト対象コードを取り込むために使ってました。

気にしてるのは、単体テストを作るコストがテストにより防止できる問題の量に見合うかってところです。

Staticをやめなくていいならテスト書いてもいいです。ただテストを書く意味があるかどうかが分からないです。

@berryzplus berryzplus force-pushed the feature/fix_mailaddress_condition branch from 7427b54 to ac814d0 Compare September 29, 2018 12:48
Copy link
Contributor

@ds14050 ds14050 left a comment

Choose a reason for hiding this comment

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

pull #421 とは違い高速化を標榜しないこの PR で IsMailAddress が計算量 O(1) を指向しているのは、予想外ですが嬉しいです。

1点確認です。a@v.v.v.v.(長さ制限を丁度超えない程度の長さを省略).v.v.v.v.v.v という、メールアドレスとしてはちょっとだけ長すぎる文字列があったとして、末尾の .v.v だけをちょん切ってそれより前をメールアドレスとして判定することをどう捉えるかということです。ちょん切られるのは -vvv というケースもあります。ドットかハイフンで始まる部分が都合良く切られます。

assert(ppszDotOrNotAlnum != nullptr);

// サブドメインの後ろに '.' があるかチェックする
if (**ppszDotOrNotAlnum != L'.') {
Copy link
Contributor

@ds14050 ds14050 Sep 29, 2018

Choose a reason for hiding this comment

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

**ppszDotOrNotAlnum*pszScanEnd と等価な場合があると思いますが、pszScanEnd はアクセスしていい範囲の外では?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sz=null-terminated string なのでアクセスしてはいけない範囲にならない認識です。

BOOL IsMailAddress( const wchar_t* pszBuf, int nBufLen, int* pnAddressLength )

pszScanEnd の最大値は pszBuf + nBufLen です。
nBufLenに正しい文字列長が渡ってきているとすればpszBuf[nBufLen - 1] == L'\0' です。
なので、直接問題にはならないコードだと思っています。

Copy link
Contributor

@ds14050 ds14050 Sep 29, 2018

Choose a reason for hiding this comment

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

pszBuf[nBufLen - 1] == L'\0' です。

pszBuf[nBufLen] にアクセスするケースを問題視しています。

sz=null-terminated string なのでアクセスしてはいけない範囲にならない認識です。

引数名に sz が付いているからってそれを信じることはできません。コンパイルすればオブジェクトコードからは消える、所詮は名前です。そして NUL で終端されていようといなかろうと、その分の +1 が nBufLen に含まれていようといなかろうと、呼び出し側が nBufLen という引数で伝える内容は、pszBuf[0] から pszBuf[nBufLen-1] までがアクセス可能であることと、C 言語の配列に関する仕様から、pszBuf+(nBufLen-1) < pszBuf+nBufLen (※たぶん nBufLen は正の数に限ると思う) という不等号が成り立つということだけです。

たぶん後ろに NUL 文字用のスペースがあるだろうからとアクセスしてもいい理由はありません。バッファ長を渡している以上、NUL で終端する必然性が呼び出し側にはありません。

さらに言えば、検索に関するコードを読んでいたときですが、文字列バッファ内に NUL 文字を含んでいる場合を考慮する旨のコメントがありました。それがバッファの長さを明示的に引数で渡す理由でもあると思います。sz なんて嘘っぱちです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

対策は要らん!というコメントを書きつつ、対策始めてました。

やっぱり問題ありますよね 😢

さらに言えば、検索に関するコードを読んでいたときですが、文字列バッファ内に NUL 文字を含んでいる場合を考慮する旨のコメントがありました。それがバッファの長さを明示的に引数で渡す理由でもあると思います。sz なんて嘘っぱちです。

サクラエディタではNUL文字が「文字列」を終端しないという事実。

ここの話とは全く関係ありませんが、改善すべき「良くない文化」と考えています。
くわしくは文字コード誤判定の対策を打つときに書くつもりでいます。

ここの話に戻すと NUL文字 は「メールアドレスに使えない文字」です。

たとえサクラエディタがNUL文字を終端扱いしないとしても、メールアドレスはNUL文字で終端されるので、NUL文字の手前までをメールアドレスとして解釈できるか否かの話になります。

Copy link
Contributor

@ds14050 ds14050 Sep 30, 2018

Choose a reason for hiding this comment

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

対策はお願いします。おそらくちょん切り問題と関係すると思います。

サクラエディタではNUL文字が「文字列」を終端しないという事実。

もはや過去の事実だろうとは思います。過去に事実であったかもやや疑っています。しかし、文字列ポインタと文字列長がセットになった BSTR 型がヌル文字を途中に含みうるように、明示的に文字列の終端を渡すインタフェイスからはそういう必然性を窺うことができます。

@berryzplus
Copy link
Contributor Author

berryzplus commented Sep 29, 2018

1点確認です。
a@v.v.v.v.(長さ制限を丁度超えない程度の長さを省略).v.v.v.v.v.v という、メールアドレスとしてはちょっとだけ長すぎる文字列があったとして、末尾の .v.v だけをちょん切ってそれより前をメールアドレスとして判定することをどう捉えるかということです。ちょん切られるのは -vvv というケースもあります。ドットかハイフンで始まる部分が都合良く切られます。

都合の悪い状況がありそうです。

前半部分「メールアドレス(255文字) .com」な場合ちぎれていいと思います思ってました。
前半部分「メールアドレス(255文字) -com」な場合ちぎれていいと 思います思ってました。
前半部分「メールアドレス(255文字) _com」な場合ちぎれていいと思います。

前半部分「メールアドレス(255文字) com」な場合ちぎれると不自然です。
単語の途中までがメールアドレス、途中から非メールアドレスになってしまいます。

@ds14050
Copy link
Contributor

ds14050 commented Sep 29, 2018

日本語が誤っていました。

ドットかハイフンで始まる部分が都合良く切られます。

と書いた意図は、解釈する側(IsMailAddress)の勝手な都合に合わせて切られる、ということでした。そのことが具合がいいという含みはなく、むしろ御都合主義に対するのと同じく改めるべきだという認識を持っていました。

前半部分「メールアドレス(255文字) .com」な場合ちぎれていいと思います。

ドメインは末尾ほど重要なので、勝手に後ろの .com をちぎってはいけないと思います。

前半部分「メールアドレス(255文字) -com」な場合ちぎれていいと思います。

-com はその直前の文字列と一体となってドメインを構成する部分なので、勝手にちぎってはいけないと思います。

@ds14050
Copy link
Contributor

ds14050 commented Sep 29, 2018

前半部分「メールアドレス(255文字) _com」な場合ちぎれていいと思います。

自分は特に言及しておらず RFC を読み込んでもいないのでスルーしましたが、アンダースコアがドメインを構成する文字ではないという事情があるのなら、ちぎれるのは問題ないと思います。あa@vあ というテキストから a@v 部分だけがメールアドレス扱いになるのと同じことですから。

@berryzplus
Copy link
Contributor Author

前半部分「メールアドレス(255文字) _com」な場合ちぎれていいと思います。

自分は特に言及しておらず RFC を読み込んでもいないのでスルーしましたが、アンダースコアがドメインを構成する文字ではないという事情があるのなら、ちぎれるのは問題ないと思います。あa@vあ というテキストから a@v 部分だけがメールアドレス扱いになるのと同じことですから。

これはそういう認識です。

対策としては、関数で検出した終端の後ろに、ドメインの一部になりうる文字が続く場合をNGとしました。これにより、途中でぶった切られたように見えることはなくなると思います。

@arigayas
Copy link

ふと、日本語ドメインを含むメールアドレスは、どうなるのか気になりました。

@KENCHjp
Copy link
Member

KENCHjp commented Sep 29, 2018

@arigayas さん、どっかでかきましたが、

けんち@けんち.みんな

なんてのもかけたかと。
実際にメールのアドレスとして流れるアドレスがマルチバイトになるわけではないでしょうけど、サクラエディタはメーラーではなくテキストエディタなので、コンバートされる前のマルチバイトメールアドレスを認識できてないとだめなのかもしれません。

https://japanese.engadget.com/2014/01/16/google/
https://octoba.net/archives/20140806-android-news-gmail.html

なので、提案したのは、もう頑張るのはやめて、
mailto:[なんかの文字]@[なんかの文字].[なんかの文字][なんかの終端文字、空白とか改行とか]
でいいんじゃねって(笑)

@arigayas
Copy link

自分で言っておいてアレですが、
日本語ドメインを含むメールアドレスには、
対応が大変そうなので早急に対応しなくても良いと思いました。

@ds14050
Copy link
Contributor

ds14050 commented Sep 30, 2018

mailto: の後ろに限れば誤検出を気にせず対応してもよさそうですね。メールソフトには日本語のままのアドレスを渡せるのでしょうか。punycode って初めて知りました。

ちなみに正規表現キーワードで /mailto:\S+@\S+/k というパターンを「URL」として色分けするように設定すると、「mailto:けんち@けんち.みんな」というテキストをダブルクリックしてメールソフトが起動できます。送信できるかまでは確かめられていません。(注意:パターンはテスト用のテキトーなもので、@ の前後で @ を除外すらしていません)

@ds14050
Copy link
Contributor

ds14050 commented Sep 30, 2018

dc3096c
スキャン範囲が余った場合のチェックを追加
・検出した終端にドメインを構成する文字が続く場合はNGにする
・ドメイン解析中の処理に終端チェックを追加

#421 (comment) で「上限より1文字だけ長いメールアドレス(候補)をいったんメールアドレスとして受け入れられるようにしておき、あとで非メールアドレスとして弾いていました」と書きましたが、上限に +1 して後で弾く理由がちょん切り対策でした。

@berryzplus
Copy link
Contributor Author

日本語メールアドレスについては考慮してないです。

けんち@けんち.みんな

こういうアドレスの場合、ゲートウェイ的には
"けんち"@(関連付けられた英語表記ドメイン or 実IP) に変換されて送信されると思っています。

逆説的にいえば、日本語ドメインに関連付けられた英語表記ドメインがあるかインターネットで問い合わせればよいだけの話なので、実現可能だと思っています。ただその前に、元のきっかけが「高速化」なので「インターネット問い合わせなんてありえん!」と思っとりました。

とりあえず高速化するつもりで変なバグ入れた状況を打破したいのが現状です。

#421 (comment)#421 (comment), メールアドレスの色替え判定を高速化すると同時にRFCに準拠させる で「上限より1文字だけ長いメールアドレス(候補)をいったんメールアドレスとして受け入れられるようにしておき、あとで非メールアドレスとして弾いていました」と書きましたが、上限に +1 して後で弾く理由がちょん切り対策でした。

結局、上限より1文字長いメールアドレスを見て判定してるのと同じですね。
手順逆ですが。

@ds14050
Copy link
Contributor

ds14050 commented Sep 30, 2018

結局、上限より1文字長いメールアドレスを見て判定してるのと同じですね。
手順逆ですが。

berryzplus さんのやり方はドメインに関する仕様が IsMailAddress に漏れてしまっていますし、DRY ではありませんよ。

@KENCHjp
Copy link
Member

KENCHjp commented Oct 1, 2018

インターネットで問い合わせればよいだけの話なので

a@a.aをメールアドレスだと思うこととそれが存在するかを検知するのは別かなと、思った点だけコメント。


// 出力値を初期化する
*ppszEndOfMailBox = nullptr;
*ppszDotOrNotAlnum = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

ヌルポインタに不用意に書き込まないように、こういう部分にこそ先駆けて assert を置くものではないでしょうか。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

詳細確認します。スタティック関数で落ちない確信があるからアサートしてないはず。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ヌルポインタに不用意に書き込まないように、こういう部分にこそ先駆けて assert を置くものではないでしょうか。

あとで見たとき関数仕様を忘れて困らないように assert 追加しました。

@berryzplus berryzplus added UnitTest and removed IMPORTANT 早急に解消すべきもの labels Oct 2, 2018
@ds14050
Copy link
Contributor

ds14050 commented Oct 3, 2018

サブドメインの文字数制限を超えないようにドメイン全体の文字数を増やしていくと assert に引っかかりました。

---------------------------
sakura
---------------------------
assert
c:\projects\sakura\sakura_core\util\string_ex.cpp(1119):
pszEndOfMailBox < pszBuf + MAX_MAILBOX + 1
---------------------------
OK   
---------------------------

@berryzplus berryzplus changed the title RFC5321解釈の誤りを訂正する [WIP] RFC5321解釈の誤りを訂正する Oct 5, 2018
@berryzplus
Copy link
Contributor Author

いったん保留にします。

自前実装で関数とテスト書いて無駄なバグ作るより、
re2c使って作業を省くほうがいい気がしてきました。
https://github.com/skvadrik/re2c/

@berryzplus
Copy link
Contributor Author

放置っぽく見えますが裏で検討進めております。

これは大枠でクリッカブルURL検出のための処理なので呼び出し元からの変更を模索してるから時間がかかってるという経緯です。

@berryzplus berryzplus changed the title [WIP] RFC5321解釈の誤りを訂正する RFC5321解釈の誤りを訂正する Jan 12, 2019
・検出した終端にドメインを構成する文字が続く場合はNGにする
・ドメイン解析中の処理に終端チェックを追加
上位からチェック処理に渡す文字列長を上限より1文字長くする。
上位ではチェック処理後に文字数のみを見て最終的な結果を判断するように変更。
追加チェックしなくても大丈夫そうなので削る
@berryzplus berryzplus force-pushed the feature/fix_mailaddress_condition branch from bdb917e to 4747b82 Compare January 12, 2019 14:14
@berryzplus
Copy link
Contributor Author

berryzplus commented Jan 12, 2019

指摘点1点 #493 (comment) に対応してコンフリクトを解消しました。

@berryzplus
Copy link
Contributor Author

CodeFactorがNGを出しているのが気になって switch 分岐を共通のテンプレ関数にできないか検討中。。。

CodeFactorにより、switch文を使う関数に対して「コード重複」と「過剰な複雑度」が検出されたことへの対策。
IsLetDig関数をやめCRTのisalnumを使うように変更。
ハングアップしたように見えた元凶はIsAtomCharの方で、@以降を判定するIsLetDlgの処理はそれほど高速でなくともよいと判断した。
@berryzplus
Copy link
Contributor Author

よし!CodeFactor黙らせた! ←ちょっと嬉しい

_In_ const wchar_t ch
) noexcept
{
return ch <= 0xFF && ::isalnum( ch );
Copy link
Member

Choose a reason for hiding this comment

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

ch <= 0xFF の条件は
https://ja.cppreference.com/w/cpp/string/byte/isalnum

で説明されている ch の値が unsigned char で表現できず、 EOF とも等しくない場合、動作は未定義です。
への対応ですか?

コードにコメントで理由の説明がほしい感じです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ch <= 0xFF の条件は
https://ja.cppreference.com/w/cpp/string/byte/isalnum

で説明されている ch の値が unsigned char で表現できず、 EOF とも等しくない場合、動作は未定義です。
への対応ですか?

コードにコメントで理由の説明がほしい感じです。

ch が ASCII文字 かつ ...
という意図で書いたものなので ch <= 0x7F の誤りです。
 ↓
2c52d35

Copy link
Contributor Author

@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.

pendingになってたコメントに気付いたので放流します...orz

_In_ const wchar_t ch
) noexcept
{
return ch <= 0xFF && ::isalnum( ch );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

某掲示板より転記です。

547名無しさん@お腹いっぱい。2019/01/22(火) 01:05:13.63ID:+KEIPYsY0

メールアドレスのisalnumなんだけどさ
厳密にいえばiswalnumを使うべきなんだよな
結果がたまたま同じになるとはいえ気持ち悪さは拭えない
ちょいまちガードコードが0xFFになってない?
それASCIIなら0x7Fの間違いじゃないかな
8ビット目突っ込むとロケール依存やで

後半部分正しい指摘と思うので修正入れます。

_In_ const wchar_t ch
) noexcept
{
return ch <= 0xFF && ::isalnum( ch );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ch <= 0xFF の条件は
https://ja.cppreference.com/w/cpp/string/byte/isalnum

で説明されている ch の値が unsigned char で表現できず、 EOF とも等しくない場合、動作は未定義です。
への対応ですか?

コードにコメントで理由の説明がほしい感じです。

ch が ASCII文字 かつ ...
という意図で書いたものなので ch <= 0x7F の誤りです。
 ↓
2c52d35

@berryzplus
Copy link
Contributor Author

これ、判断難しいようなら一旦破棄するのが現実的ですかね。
#421 含めて revert して、純粋に wcschrが遅い問題に対処 だけにしたら難しいこと考えなくてよくなる気がします。

@ds14050
Copy link
Contributor

ds14050 commented Feb 17, 2019

これ、判断難しいようなら一旦破棄するのが現実的ですかね。

異存ありません。その方がユーザーへの影響度合いが小さいと思います。

@berryzplus
Copy link
Contributor Author

#421をrevertする方法がわかり次第閉じます。

@ds14050
Copy link
Contributor

ds14050 commented Feb 18, 2019

コミットが続いていてレビューのタイミングが掴めないという事情もありそうです。一部ケースでの反応低下と、それよりありふれている(ちょっとのテストで発生する)誤判定を天秤にかけて影響度合いを判断しましたが、@berryzplus さんが「バグはもうない。あとはレビューだけだ」と考えるならレビューを待つ手もあると思います。

根本的な話をすると、エディタで行うアドレス判定の価値は「最小の手間で最大の効果を期待するコストパフォーマンス」と「正規表現キーワードでは実現できない厳密さ」のどちらにもあると思いますが、個人的な話をすれば、誤判定に悩まされたことがないので厳密さを求める立場にはありません。

@ds14050
Copy link
Contributor

ds14050 commented Feb 18, 2019

あ、リリースを考えているから待てないんですね。その判断は正しいと思います。

@berryzplus
Copy link
Contributor Author

CSVやJSONなどのデータ規格と比べてメールアドレスの仕様は雑なので、厳密に準拠することのメリットはほぼないと思われます。もともと「RFC規格に対応させたい」という強い意思でやってたわけでもないので、仕切り直したほうがシンプルな議論ができる気がしています。(クリッカブルURLはどうだったらいいんだろう?という議論が要ると思ってます。

can not revert automatically 的なメッセージが出てたのでrevertは手動でやるしかなさそうです。

@berryzplus
Copy link
Contributor Author

berryzplus commented Feb 23, 2019

replaced by #792.

@berryzplus berryzplus closed this Feb 23, 2019
@berryzplus berryzplus deleted the feature/fix_mailaddress_condition branch February 23, 2019 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working) UnitTest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants