Add tests of IsMailAddress. / IsMailAddress のテストを追加します。

[ds14050]

IsMailAddress の offset パラメータを正数、０、負数と変化させてテストします。同時に関数コメントを整備しました。

関連 Issue: #811 「引数が 4つのバージョンの IsMailAddress の単体テストを追加
する」

[m-tmatma]

• PR のタイトルを見ただけで何をするのかが明確になるようにしたほうがいいです。
• IsMailAddress の仕様に関するドキュメントがないので仕様を明文化してほしいです。
• 単体テストを実装する場合は、どういう観点でテストするのかももう少し詳しく書いたほうがいいです。

[m-tmatma]

* IsMailAddress の仕様に関するドキュメントがないので仕様を明文化してほしいです

これに関しては別 PR でもいいですが、
どういう動作が期待値なのか、明確になっていないと単体テスト、実装の正当性が
確認できないです。

[ds14050]

IsMailAddress と IsURL に関数コメントを追加しました。> 1ac0656

[ds14050]

PR のタイトルと本文を修正しています。

[m-tmatma]

このコメントどういう意味ですか?

[ds14050]

テストケースの読み方を教える親切心からのコメントです。

[m-tmatma]

この英語の意味がわかりません。
どういうことか日本語で教えてください。

[ds14050]

なんちゃって英語を否定されるのはつらいですね。反論できない。

「Buffer+1 はメールアドレス(の先頭)を指すアドレスだから IsMailAddress が TRUE を返すのが期待値で問題ない」「Buffer+2 はメールアドレスの先頭ではない(直前にメールアドレスの構成文字がある)から IsMailAddress が FALSE を返すのが期待値で問題ない」「正のオフセットが与えられなかったときは直前の文字を読むわけにはいかないから境界判定を加味したメールアドレス判定ができないので Buffer+2 が TRUE と判定されるのは仕方がない」という意味です。

[m-tmatma]

英語で書くかは対象読者が誰なのか次第ですが、
ここで英語でコメント書いてもここを英語で書いてもメリットないように思います。

「Buffer+1 はメールアドレス(の先頭)を指すアドレスだから IsMailAddress が TRUE を返すのが期待値で問題ない」「Buffer+2 はメールアドレスの先頭ではない(直前にメールアドレスの構成文字がある)から IsMailAddress が FALSE を返すのが期待値で問題ない」「正のオフセットが与えられなかったときは直前の文字を読むわけにはいかないから境界判定を加味したメールアドレス判定ができないので Buffer+2 が TRUE と判定されるのは仕方がない」という意味です。

これを日本語でコメントに追加したらいいと思います。

[berryzplus]

日本語よりも英語の方がC言語に近いので、制限事項の類は英語で書いたほうがよいと思っとります。

それはそれとして、以下のように「こう解釈したんだけど合ってる？」と訊いたらよいように思いました。

// 原文
// Limitation: Non positive offset prevents
// IsMailAddress from looking behind
// of a possible head 
// of a mail address.

// Google先生の翻訳結果
// 制限事項: 正でないオフセットは、IsMailAddressが
// メールアドレスの先頭の可能性のある後ろを
// 見ないようにします。

// おいらの解釈
// 制限事項: 負のオフセットを指定すると、
// メールアドレスの先頭を戻り読みしません。

厳密には違うんだろうけど、これで合ってる？と。

[berryzplus]

というか、index = 0 に見えるパラメータの説明に、負数を渡したときの制約が書かれているのは何故だ・・・。

[ds14050]

prevent/keep A from Bing で A が B するのを妨げるという意味だったと、高校生(中学生？)の頃の記憶が言っています。

オフセット値が正でない限り(ゼロでも無理)、pszBuf + offset がメールアドレスの先頭に見えたとしても、本当にそれが先頭かどうか戻り読みして確かめることはできない、と言っとります(そのつもり)。戻り読みして確かめたいけど、オフセットが正でないからできないんだ、と。

つまり、berryzplus さんの解釈は間違っていませんし、0 オフセットに制限に関する注釈があるのも間違っていません。

[ds14050]

positive/negative offset と書かずに Non positive offset と書いているのは、0 を含めるためなのです。

[berryzplus]

納得！positiveの反対語negativeを使わずに、あえてnon positiveとした理由と、offset=0に制約の説明がついていた理由に合点がつきました。

[m-tmatma]

負の offset が与えられたときの動作はどうなりますか?

[ds14050]

正のオフセットに関する文意を否定してください。助詞「は」の用法のひとつです。

[m-tmatma]

?

[ds14050]

正のオフセットも負のオフセットも特段の区別はありません。書かれていることが為され、書かれていないことは為されません。

その違いはメモリを読み取ることができる範囲に制限があるために境界判定ができる場合とできない場合があることに由来します。関数の仕様よりも基礎的な部分から生じる当然の帰結です。

[m-tmatma]

負の値が想定外なので常に FALSE を返すとか、
0 の値が渡さたれら渡たれさたバッファより前の文字は保証できないので
戻り値が正しくないかもしれないとか、どのような引数を渡したときに
どのような動作をするのかを仕様として規定してほしいという話です。

[m-tmatma]

正しい結果を得るためにどういうパラメータを与えるべきかはもう書かれています。

負のオフセットは許容しているだけで推奨しているわけではありません。

正しい使い方をしなかった場合にどのような動作をするのかを問うています。

負のオフセットを渡す場合に動作を保証しないという仕様なら
221行目で -1 を渡すテストケースがありますが、
結果の確認は意味がないことになります。

以下のような対応が考えられると思います。

1. エラーとしてはじく (常に FALSE を返す)
2. 仕様を明示する
3. 結果は保証外なのでテストケースから外す (あるいは戻り値のチェックはしない)

[ds14050]

負のオフセットは想定して許容しています。その想定の通りになっているかをテストによって確認しています。想定結果とは、従来の offset パラメータを取らなかった IsMailAddress に第一パラメータとして pszBuf + offset を渡したのと同じ結果です。その仕様は明示されています。

[ds14050]

その仕様は明示されています。

思い込みによってこう考えているとしたら、どこに曖昧さがあるかを教えてもらうことで対応できるかもしれません。

しかしそれが「正しい使い方をしなかった場合にどのような動作をするのかを問うています。」だとすれば、そのひとつ前に答えがあります。「それに反したときに何が行われず結果的にどういう誤った結果が返ってくるかも具体的に書かれています。」

気がつけばこれは最初の疑問です。わからないはずがないと考えたのが間違いだったのでしょうか。

「正の offset が与えられた場合は判定開始位置直前の文字との間で境界判定を行います。」で不十分ならば、「正の offset が与えられた場合は、その場合に限り、判定開始位置直前の文字との間で境界判定を行います。」とすればより伝わりますか。

[m-tmatma]

「それに反したときに何が行われず結果的にどういう誤った結果が返ってくるかも具体的に書かれています。」

ソースコードを見ればわかりますが、仕様としては明示されていない認識です。
そのうえで単体テストのテストケースにあったので、ん? と思いました。

仕様的には、負の場合の動作は保証しませんというのでもありだとは思いますが、
渡すことが可能なので、仕様を定義したうえでドキュメント化したいです。

まあ、そんなにこだわらなくてもいいのかもしれませんが。

[ds14050]

すべて IsMailAddress の doxygen コメントからの引用です。

@param[in] offset メールアドレス判定開始文字を示す、pszBuf からの相対位置。

int 型の offset が表す相対位置に関して向き(符号)を制限していません。

正の offset が与えられた場合は、その場合に限り、判定開始位置直前の文字との間で境界判定を行います。

負のオフセットを与えた場合には境界判定が行われないことがわかります。

途中から切り出したメールアドレスの一部をメールアドレスであると誤って判定しないために
pszBuf を固定し offset を０以上の範囲で変化させるのが望ましい使用方法です。

負のオフセットを与えたときに、メールアドレスの途中からを指してメールアドレスであると判定してしまうことがあるという具体的情報と指針です。このくだりは蛇足ですが親切のためにあります。

負のオフセットを与えた場合に何が違うのかといえば、境界判定が行われないとすでに示しています。

以上をもって、何が解らないのかがわからないのです。

[m-tmatma]

前半のループ(226行目～228行目)と後半のループ(233行目～239行目)をくっつけてしまっていいと思います。

[ds14050]

最初はそうしていましたがわけました。前半のループはテストケースのバリデーションが目的であり、後ろのループとはたまたま構造が一致しているに過ぎません。パフォーマンスを気にする場面でもないと考えました。

[m-tmatma]

パフォーマンスというより、くっつけたほうが、シンプルになるし見やすいと思います。

[ds14050]

目的が異なるのです。assert が IsMailAddress のテストとは直接関係しないから、惑わされないように読み解きに無駄な時間を使わないで済むように、分けているのです。

[m-tmatma]

この assert はテストを実施するための前提条件を確認していると思います。

わかることで惑わされることがないというよりは、長くなるし
逆にわかりづらくなると思います。

[ds14050]

assert はテストケースを制約する定義の一部です。指摘により「1.テストケースの準備」「2.テストケースへの IsMailAddress の適用」という構成が破壊されます。

[ds14050]

時間があったので、パラメータの値を２つ３つ選んで与えるのではなく、一定の定義域の中で３つのパラメータを網羅的に動かしてテストしてみました。

追記 x64 ビルドがコンパイルエラーになったのでワークアラウンド(static_cast)を入れました。根治策は IsMailAddress の int パラメータを ptrdiff_t にすることでしょうか。

[m-tmatma]

CodeFactor が複雑さを指摘していますが、もっとシンプルになりませんか?
また、何をテストするのかのコメントが欲しいです。

[ds14050]

いろいろなもののスコープをひとつの関数に閉じ込めているためにそういう指摘に繋がっています。共有しないものをファイルスコープにばらまくのもわかりにくいでしょう。

[ds14050]

それに CodeFactor はこれがテストコードだという事情、ファイル構成・関数構成の特殊事情も承知していないでしょう。

[ds14050]

見落としていました。

何をテストするのかのコメントが欲しいです。

テスト対象は「TEST(testIsMailAddress, OffsetParameter)」によって示しています。オフセットパラメータを可変させてテストケースを定義していることからも明らかです。コメントはソースコードの補助であり、独立したドキュメントではありません。

[m-tmatma]

具体的にどのようなテストをするのかは明らかではないです。
概略ぐらいしかわかりません。

[berryzplus]

やりとりをみて、レビューをされるのにあんまし慣れてないのかな？と思いました。

業務でやる場合も「こうすれば絶対大丈夫！」というセオリーがあるわけではないので、うまくやる方法はみんなで考えていけばいいと思ってます。(ある程度のセオリーは存在するのでそれは活用していきたいです。)

テストコードのレビューには、一般PGよりもコストの高い人材が必要なので業務現場でテストコードがレビューされることはほとんどないと思います。通常、単体テストのレビューは、詳細仕様から起こすテスト仕様に対して行います。

今回のケースで行くと「テスト対象コードにあるコメントが詳細仕様」にあたり、「テストコードにあるコメントがテスト仕様」にあたります。コメントの書きっぷりにしつこくコメント付いているのは「普通そうする」のセオリーに照らして自然だと思います。テスト仕様が分かりにくければ詳細仕様と突き合わせできない、つまり「レビューできない」ってことになるからです。

仕様レベルですべての条件がテストされていればOKと考える、これが一般的なテスト仕様のレビューです。

テストコードのレビューになるとまた違った観点でレビューを行うことになるのですが、 めんどくさいからそこまでやらんでもよくね？ とぼくは思っとります。(一部の超重要なコアクラスのテストを除いては。)

仕様レビューで「コード見て」と発言するのは、コードレビューへの移行提案を意味すると思います。コードレビューで判断するほうがハードだと思うので、可能な限り仕様レビューでお茶を濁したいです。

[ds14050]

m-tmatma さんと berryzplus さんのお二人は(たぶん KENCHjp さんもかな)自分の知らない共通知識を持っているみたいですね。

「詳細仕様としてのテスト対象コードにあるコメント」というのは、IsMailAddress の宣言部のコメントであろうと思います。無関係ではないと思ったからこそこの PR でまとめて整備しました。

「テスト仕様にあたるテストコードにあるコメント」というのは自分が頑なにコメントすることを拒んでいる部分だと思います。何をテストしようとしているかではなく、何がテストされているかにしか意味がない、と。また m-tmatma さんはこの部分に概略だけでなく設計や意図を求めています。しかし自分には offset パラメータが機能しているか確かめたい、という意図しかない。

これはある１つの副作用のない関数のテストですから、パラメータとして何を与え何が返ってくるか、そして何が返ってくるべきか、しか見るところがありません。そのすべてがテストケースの定義に凝縮されています。これ以上の何を求められているかがわからないのです。

[ds14050]

「違った観点」のリンク先を見ました。ホワイトボックステストは無駄が多くてやってられないというのが感想です。リファクタリングひとつで無駄になってしまいます。

インタープリタ型の言語であれば、実行パスに入るまでシンタックスエラーや未定義変数の参照みたいな根本的なミスでさえ発覚しないという事情がありますから、コードカバレッジにこだわることに特有の意味がありますが、C++ ではそのレベルのミスはコンパイラが見つけるので動機が薄れます。

[ds14050]

berryzplus さんが書きました「違った観点」についてもう少し。そこでのキーワードはホワイトボックステストとコードカバレッジでした。

IsMailAddress の実装が仮に、引数がこういう値(たとえば負の数)の場合はこういう答えを返す、こういう場合は無効なのでこういう答えを返す、というように大きな分岐によって実装されていたとします。これは、IsMailAddress が実質的に、不連続な複数の「関数」の寄せ集めによって定義・実装されているということを意味します。

このような実装の IsMailAddress に対して、それぞれの「関数」(分岐)に突入するようなパラメータを漏れなく選んでテストするということが、ホワイトボックステストに求められることであり、berryzplus さんが「そこまでやらんでもええんじゃね」と言っている部分だと理解しました。

ところで、自分は「負のオフセットを特別扱いしない」と書きました。IsMailAddress の全体がそのように実装できているかは確認が必要だとしても、その発言の根っこにある意図は、IsMailAddress を連続な１つの関数として定義し実装するということです。

分岐、スペシャルケース、関数定義の分裂、どれも同じ意味ですが、これらを避けて関数を１つの定義とコードで実装できれば、ホワイトボックステストが網羅すべき分岐は目に見えなくなります。

それでも符号の入れ替わりや境界値の扱いなど、プログラミングエラーが入りやすい、定義域の部分はあると思います。テストケースはそれを試すために定義しましたが、その選び方に正解はなくまったく恣意的なもので、説明が困難なものです。

[ds14050]

細かい話なので理解は求めませんが、offset より小さい nBufLen をパラメータとして受け入れるということと、offset, nBufLen を１つの定義(コード)で実装することは両立しません。

STL において begin()/end() が返すイテレータと rbegin()/rend() が返すリバースイテレータの実装が異なるのと同じ理由と結果です。

<追記> STL ではイテレータが異なる実装を取ることでアルゴリズムの定義が１つで済んでいます。IsMailAddress のパラメータはただのポインタなので、IsMailAddress の中で分岐したコードが必要になります、本来は。 </追記>

気にしなくても大丈夫です。IsMailAddress にとってその領域はすべて FALSE になるだけです。

[m-tmatma]

このループにインデントがないです。

[ds14050]

インデントは不要です。

[ds14050]

Python の内包表記だと for x in xx for y in yy という風に複数の繰り返しを並べられるんですね(検索しました)。

Scala でも for (i <- 1 to 2; j <- 1 to 3) という風に複数の繰り返しを並べられるようです。

C++ の for 文ではこうなるということです。

[m-tmatma]

普通 c++ では ループの中に ループを入れる場合はインデントしますし
かっこで囲むのが、ベストプラクティスですし、
このようにインデントなしで書くと、一目で階層構造が見えません。

[ds14050]

階層構造は存在していません。どの３つ２つの for ループも入れ替えが可能です。

[berryzplus]

自宅待機でヒマなので3本目。

	/*
	   Text...TextEnd の範囲の文字配列に対して、IsMailAddress の３つの
	   引数(pszBuf, offset, nBufLen)がとりうるすべての値を総当たりで試す。
	*/

そもそもの話、offsetやnBufLenが負数を受け入れる必要ってあるんでしょうか？

インデントの話は、ぼくは分かるからこれでもいいです。

C言語にはif文というのがありますが、else if文というのはないんです。

いかなる場合も階層構造を維持すべきだ、とするなら以下の用に書くのが正しいはずです。

if (a == 1) {
    // a == 1
} else if (a == 2) {
        // a == 2
    } else if (a == 3) {
            // a == 3
        } else {
            // 3 < a
        }

こんなコード書くヤツいねぇYO！
・・・って書こうと思ったけど、たまにいますね 😄
ただ、標準的には else if と書くもんだと思っています。

指摘されているコードで、p1,p2,p3は独立した変数ではなく wchar_t* values[3] = {p1,p2,p3}; だと思うんです。

wchar_t* valuesSet[文字列長の3乗][3]が定義できるとすれば、for (wchar_t* values[3] : valuesSet) と書けるコードだと思います。(文法的にダメ？ｗ)

データ構造が適切かどうかはともかくとして、テストデータを生成するための 3連while文 の重要ポイントは、階層構造になってることではなくて、同じ式で生成した3つの値を詰めたパラメータを作ってるということだと思います。

なのでまぁ、ここで気になるのは 本当に負数を受け入れる仕様が必要なんでしたっけ？ のほうで、ここまでの全部の議論を見てたわけじゃないから「何をいまさら」と一蹴されるの覚悟で書き込んでみたわけです。

[ds14050]

wchar_t* valuesSet[文字列長の3乗][3]が定義できるとすれば、for (wchar_t* values[3] : valuesSet) と書けるコードだと思います。(文法的にダメ？ｗ)

一番最初に

for (auto xyz: X × Y × Z) // X(,Y,Z) は x(,y,z) の集合。× は直積。

のように書きたいと考え、聞きかじりの知識
で std::iota というのが使えるのではないかと検索しました。これは数列を生み出すジェネレータとしては使えず、配列を埋めるものでした。

C++ ではジェネレータはイテレータの形をとるのかもしれないと、標準ライブラリのリファレンスを眺めても、答えは見つかりませんでした。

追記@2019-04-18 考えることは同じか！「コルーチンもyield文もないけどpythonのジェネレータが欲しいのでイテレータで頑張る」

[ds14050]

負数を受け入れる仕様は従来からのものなので関知していません。そういう値に対して IsMailAddress は無難に FALSE を返しているみたいです、ということがテストにより明らかになりました。

IsMailAddress のパラメータの型について。

自分は鬼雲の API のように、offset も nBufLen もポインタで渡すのがいいと思っています。ちょうどテストコードの p1, p2, p3 がそのまま渡せるように。

結局 IsMailAddress が必要とする値、有効でないといけない値、アクセスするアドレスは pszBuf + offset や pszBuf + nBufLen (- 1) なのですから、そのままその値を渡せばいいと考えます。

IsMailAddress の現在の仕様のように差分を値にしてしまうと、加算してオーバーフローしてしまうような pszBuf と nBufLen の組を与えることができてしまいます。そういう無効なパラメータの組に対して責任を負うのが IsMailAddress か、その呼び出し側か、はたまたどちらも無責任なのか。考えなければいけないことが増えます。

呼び出し側がパラメータの有効性を保証することを考えます。「加算した結果が有効」よりも「有効なポインタ」であることを保証する方が直接的です。そしてその値がそのまま受け渡せる関数仕様であってほしいと考えます。

しかし IsMailAddress はすでに存在している関数なので、できるのは int を ptrdiff_t にすることかな、と> #823 (comment) 。size_t ではないな、と。

[ds14050]

(文法的にダメ？ｗ)

「C++17では構造化束縛が追加された」らしいです。

追記 ３つ組タプルを使えば構造化束縛はなくても構いませんし、何より普通に下の通りに書けました。あえて型を明示しましたが auto で大丈夫です。

int int3[][3] = { {1,2,3}, {10,20,30}, {100,200,300} };
for (int(&abc)[3]: int3) {
	std::cout << abc[0] << ',' << abc[1] << ',' << abc[2] << std::endl;
	// 1,2,3
	// 10,20,30
	// 100,200,300
}

[ds14050]

wchar_t* valuesSet[文字列長の3乗][3]が定義できるとすれば

配列を確保するのは、そうするならまさに先ほど挙げた iota の出番なわけですが、あまりに富豪的なので、0 から文字列長の３乗未満の整数について繰り返してそこから p1, p2, p3 を割り算や剰余演算(それと Text の加算)を使って取り出すことは考えました。for ループはひとつでインデントは不要になります。

しかしこれではただ意地になっている人です。利がありません。

[ds14050]

コメントを拡充しました。IsMailAddress の offset 値による違いが関数コメントにおいて明示され、テストについては見通しが良くなったと思います。

思えば m-tmatma さんの最初の要求も「負のオフセットの扱いを明示してほしい」でした。しかし負のオフセットに直接的に言及し、結果的に一端の立場を与えることになるのでは逆効果で、コメントしない方がマシというものです。負のオフセットを与えようというひねくれ者に必要以上に親切にしてもいいことがありません。必要なだけの情報はありました。

今回コメントを修正したのは、一連のやり取りの中で、正のオフセットにだけ言及しながら簡潔に負のオフセットを除外する言い回しを思い付いたからです> #823 (comment) 。おかげさまで前の版より文意が明らかになったと感じています。より明らかになったということは、以前はそれほど明らかではなかったのでしょう。

[m-tmatma]

まあいいことにします。
対応ありがとうございます。

[m-tmatma]

codefactor の警告の無効化方法わからん。