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

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

Merged
merged 5 commits into from
Apr 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions sakura_core/parse/CWordParse.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,40 @@ class CWordParse{
static bool _match_charlist( const WCHAR c, const WCHAR *pszList );
};

BOOL IsURL( const wchar_t* psz, int offset, int length, int* outLength);/* offset 引数の追加により境界判定が行える高速版 */
/** 指定アドレスが URL の先頭ならば TRUE とその長さを返す。
@param[in] pszLine 文字列バッファの先頭アドレス
@param[in] offset URL 判定開始文字を示す、pszLine からの相対位置。
@param[in] nLineLen URL 判定最終文字の次を示す、pszLine からの相対位置。
@param[out] pnMatchLen URL の長さを受け取る変数のアドレス。NULL可。長さとは pszLine + offset からの距離。

境界判定はメールアドレスの先頭でのみ行われ、URL の先頭ではこれまで通り行われません。
*/
BOOL IsURL( const wchar_t* pszLine, int offset, int nLineLen, int* pnMatchLen);

/** @deprecated 互換性のために残されています。offset 引数が追加されたものを使用してください。
*/
inline
BOOL IsURL( const wchar_t* psz, int length, int* outLength) /* 指定アドレスがURLの先頭ならばTRUEとその長さを返す。高速版の追加により obsolete. */
BOOL IsURL( const wchar_t* pszLine, int nLineLen, int* pnMatchLen)
{
return IsURL(psz, 0, length, outLength);
return IsURL(pszLine, 0, nLineLen, pnMatchLen);
}
BOOL IsMailAddress( const wchar_t* pszBuf, int offset, int nBufLen, int* pnAddressLength); /* offset 引数の追加により境界判定が行える高速版 */

/** 指定アドレスがメールアドレスの先頭ならば TRUE とその長さを返す。
@param[in] pszBuf 文字列バッファの先頭アドレス
@param[in] offset メールアドレス判定開始文字を示す、pszBuf からの相対位置。
@param[in] nBufLen メールアドレス判定最終文字の次を示す、pszBuf からの相対位置。
@param[out] pnAddressLength メールアドレスの長さを受け取る変数のアドレス。NULL可。長さとは pszBuf + offset からの距離。

正の offset が与えられた場合は、その場合に限り、判定開始位置直前の文字との間で境界判定を行います。
途中から切り出したメールアドレスの一部をメールアドレスであると誤って判定しないために
pszBuf を固定し offset を0以上の範囲で変化させるのが望ましい使用方法です。
*/
BOOL IsMailAddress( const wchar_t* pszBuf, int offset, int nBufLen, int* pnAddressLength);

/** @deprecated 互換性のために残されています。offset 引数が追加されたものを使用してください。
*/
inline
BOOL IsMailAddress( const wchar_t* pszBuf, int nBufLen, int* pnAddressLength) /* 現在位置がメールアドレスならば、NULL以外と、その長さを返す。高速版の追加により obsolete. */
BOOL IsMailAddress( const wchar_t* pszBuf, int nBufLen, int* pnAddressLength)
{
return IsMailAddress(pszBuf, 0, nBufLen, pnAddressLength);
}
Expand Down
117 changes: 117 additions & 0 deletions tests/unittests/test-is_mailaddress.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@

#define NOMINMAX
#include <tchar.h>
#include <wchar.h>
m-tmatma marked this conversation as resolved.
Show resolved Hide resolved
#include <assert.h>
#include <string>
#include <Windows.h>
#include "parse/CWordParse.h"

Expand Down Expand Up @@ -199,6 +202,120 @@ TEST(testIsMailAddress, CheckAwithAtmark)
ASSERT_SAME(FALSE, szTest, _countof(szTest) - 1, NULL);
}

TEST(testIsMailAddress, OffsetParameter)
{
/*
Prepare test cases.

3つの offset値(-1, 0, 1)と、メールアドレスに見える2つの文字列
(Buffer+1=メールアドレスの先頭, Buffer+2=メールアドレスの途中)
の組み合わせにより定義する。
*/
const wchar_t* const Buffer = L" [email protected]";
const wchar_t* const BufferEnd = Buffer + wcslen(Buffer);
const struct {
bool expected;
const wchar_t* address; // to be tested by IsMailAddress.
int offset; // passed to IsMailAddress as 2nd param.
const wchar_t* buffer() const { // passed to IsMailAddress as 1st param.
return this->address - this->offset;
}
} testCases[] = {
{ true, Buffer+1, 0 }, // true is OK. Buffer+1 is a mail address.
{ true, Buffer+1, 1 }, // true is OK. Buffer+1 is a mail address.
{ true, Buffer+1, -1 }, // true is OK. Buffer+1 is a mail address.
{ true, Buffer+2, 0 }, // Limitation: Non positive offset prevents IsMailAddress from looking behind of a possible head of a mail address.
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.

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

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

@ds14050 ds14050 Apr 8, 2019

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 17, 2019

Choose a reason for hiding this comment

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{ false, Buffer+2, 1 }, // false is OK. Buffer+2 is not a head of a mail adderss.
{ true, Buffer+2, -1 } // Limitation: Non positive offset prevents IsMailAddress from looking behind of a possible head of a mail address.
};
for (auto& aCase: testCases) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

Copy link
Member

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.

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

assert(Buffer <= aCase.buffer());
}

/*
Apply IsMailAddress to the cases.
*/
for (auto& aCase: testCases) {
EXPECT_EQ(
aCase.expected,
bool(IsMailAddress(aCase.buffer(), aCase.offset, BufferEnd - aCase.buffer(), NULL))
) << "1st param of IsMailAddress: pszBuf is \"" << aCase.buffer() << "\"\n"
<< "2nd param of IsMailAddress: offset is " << aCase.offset;
}
}

TEST(testIsMailAddress, OffsetParameter2)
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

見落としていました。

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

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

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

Choose a reason for hiding this comment

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

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

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

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

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 18, 2019

Choose a reason for hiding this comment

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

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

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

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

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

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 18, 2019

Choose a reason for hiding this comment

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

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

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

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

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

{
const wchar_t* const Text = L" [email protected] ";
const wchar_t* const Address = Text + 3; // Address is "[email protected]"
const wchar_t* const PseudoAddress = Text + 6; // PseudoAddress is "[email protected]", shortest form recognized by IsMailAddress.
const wchar_t* const PseudoAddressEnd = Text + 17;
const wchar_t* const AddressEnd = Text + 19;
const wchar_t* const TextEnd = Text + 22;

struct Result {
bool is_address;
int length;
};
const Result FalseResult = {false, 0};
auto ExpectedResult = [=](const wchar_t* p1, const wchar_t* p2, const wchar_t* p3) -> Result
{
/*
p2 と p3 が以下の条件を外れたら、TRUE 判定の可能性はゼロ。
* Address <= p2 <= PseudoAddress
* PseudoAddressEnd <= p3
*/
if (p2 < Address || PseudoAddress < p2) {
return FalseResult;
}
if (p3 < PseudoAddressEnd) {
return FalseResult;
}

if (p2 == Address) {
if (AddressEnd <= p3) {
return Result{true, static_cast<int>(AddressEnd - Address)}; // 文句なしの TRUE 判定。
} else {
return Result{true, static_cast<int>(p3 - Address)}; // アドレスの終端が切り詰められているが、IsMailAddress には知る由がない。ゆえに問題なし。
}
} else if (p2 <= p1) {
if (AddressEnd <= p3) {
return Result{true, static_cast<int>(AddressEnd - p2)}; // アドレスの先端が切り詰められているが、IsMailAddress には知る由がない。ゆえに問題なし。
} else {
return Result{true, static_cast<int>(p3 - p2)}; // アドレスの先端と終端が切り詰められているが、IsMailAddress には知る由がない。ゆえに問題なし。
}
} else {
return FalseResult; // アドレスの先端が切り詰められ、IsMailAddress が境界判定によりそれを検知した。文句なしの FALSE 判定。
}
};
auto IsEqualResult = [](const Result& expected, const Result& actual) -> testing::AssertionResult
{
if (expected.is_address != actual.is_address) {
return testing::AssertionFailure() << "IsMailAddress returned " << (actual.is_address?"TRUE":"FALSE") << " but expected " << (expected.is_address?"TRUE":"FALSE") << ".";
}
if (expected.length != actual.length) {
return testing::AssertionFailure() << "IsMailAddress returned the address length " << (actual.length) << " but expected " << (expected.length) << ".";
}
return testing::AssertionSuccess();
};

/*
Text...TextEnd の範囲の文字配列に対して、IsMailAddress の3つの
引数(pszBuf, offset, nBufLen)がとりうるすべての値を総当たりで試す。
*/
for (const wchar_t* p1 = Text; p1 != TextEnd; ++p1) // p1 is a pointer to buffer.
for (const wchar_t* p2 = Text; p2 != TextEnd; ++p2) // p2 is a pointer to address.
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.

インデントは不要です。

Copy link
Contributor Author

@ds14050 ds14050 Apr 14, 2019

Choose a reason for hiding this comment

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

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 14, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

	/*
	   Text...TextEnd の範囲の文字配列に対して、IsMailAddress の3つの
	   引数(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) と書けるコードだと思います。(文法的にダメ?w)

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 17, 2019

Choose a reason for hiding this comment

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

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

一番最初に

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

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

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 17, 2019

Choose a reason for hiding this comment

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

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

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

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

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

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

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

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

Copy link
Contributor Author

@ds14050 ds14050 Apr 17, 2019

Choose a reason for hiding this comment

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

(文法的にダメ?w)

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

追記 3つ組タプルを使えば構造化束縛はなくても構いませんし、何より普通に下の通りに書けました。あえて型を明示しましたが 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
}

Copy link
Contributor Author

@ds14050 ds14050 Apr 17, 2019

Choose a reason for hiding this comment

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

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

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

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

for (const wchar_t* p3 = Text; p3 != TextEnd; ++p3) { // p3 is a pointer to the end of buffer.
Result actual = {false, 0};
actual.is_address = IsMailAddress(p1, p2 - p1, p3 - p1, &(actual.length));

EXPECT_TRUE(IsEqualResult(ExpectedResult(p1, p2, p3), actual))
<< "1st param of IsMailAddress: pszBuf is \"" << (p1 <= p3 ? std::string(p1, p3) : "") << "\"\n"
<< "2nd param of IsMailAddress: offset is " << (p2 - p1) << "\n"
<< "pszBuf + offset is \"" << (p2 <= p3 ? std::string(p2, p3) : "") << "\"";
}
}

//////////////////////////////////////////////////////////////////////
// テストマクロの後始末

Expand Down