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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e8ce34
Add tests of IsMailAddress.
ds14050 Mar 31, 2019
58bd7fe
Add function comments to IsMailAddress/IsURL.
ds14050 Mar 31, 2019
dc18725
more tests.
ds14050 Apr 13, 2019
720727e
Fix alignment.
ds14050 Apr 13, 2019
c9f39c0
comment.
ds14050 Apr 15, 2019
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
View file
Open in desktop
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 が与えられた場合は判定開始位置直前の文字との間で境界判定を行います。
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

途中から切り出したメールアドレスの一部をメールアドレスであると誤って判定しないために
pszBuf を固定し offset を０以上の範囲で変化させるのが望ましい使用方法です。
*/
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
40 changes: 40 additions & 0 deletions tests/unittests/test-is_mailaddress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

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

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

TEST(testIsMailAddress, OffsetParameter)
{
/*
Prepare test cases.
*/
const wchar_t* const Buffer = L" test@example.com";
const wchar_t* const BufferEnd = Buffer + wcslen(Buffer);
const struct {
bool expected;
m-tmatma marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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 •
edited
Loading

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());
}

/*
Run tests.
*/
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;
}
}

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

Expand Down