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

循環構造をサポートするdeep-equal関数 #460

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

marihachi
Copy link
Contributor

@marihachi marihachi commented Nov 7, 2023

What

循環構造をサポートするdeep-equalを実装。
連想配列で必要になりそうなのでひとまず追加。
標準ライブラリ等でも使えそう。

少し怪しい部分があるので考えたい

Why

Additional info (optional)

@marihachi
Copy link
Contributor Author

良さそう

@marihachi marihachi marked this pull request as ready for review November 7, 2023 13:41
@marihachi marihachi requested a review from syuilo November 7, 2023 13:41
@marihachi marihachi self-assigned this Nov 7, 2023
@marihachi marihachi mentioned this pull request Nov 7, 2023
@marihachi
Copy link
Contributor Author

marihachi commented Nov 7, 2023

constructorとか怪しいかも
あと特殊なやつってあったっけ

(Mapなどではない)通常のオブジェクトに任意のキーでユーザーがアクセスすることがなければ大丈夫そうかも

@FineArchs
Copy link
Member

equal({ a: 1 }, { a: 1, b: 2 })

のように、オブジェクト同士の比較で後者が前者の要素全部+αを持っている時に偽陽になってしまうようです

@FineArchs
Copy link
Member

怪しいのというと

let a, b, c;
a = { b };
b = { c };
c = { a };
let x = { a };
let y = { a: { b } };
equal(x, y); // false

が微妙ですね

@marihachi
Copy link
Contributor Author

怪しいのというと

let a, b, c;
a = { b };
b = { c };
c = { a };
let x = { a };
let y = { a: { b } };
equal(x, y); // false

が微妙ですね

これ修正されてませんか?

@marihachi

This comment was marked as outdated.

@marihachi
Copy link
Contributor Author

marihachi commented Nov 7, 2023

あ、テストケースが間違ってますね

a = { b };

の時点ではbはundefinedです
多分

@marihachi
Copy link
Contributor Author

marihachi commented Nov 7, 2023

これだと動きました

let a: any, b: any, c: any;
a = { b };
b = { c };
a.b = b;
c = { a };
b.c = c;
let x = { a };
let y = { a: { b } };
console.log(deepEqual(x, y)); // true

@FineArchs
Copy link
Member

これだと動きました

let a: any, b: any, c: any;
a = { b };
b = { c };
a.b = b;
c = { a };
b.c = c;
let x = { a };
let y = { a: { b } };
console.log(deepEqual(x, y)); // true

確認できました

@FineArchs
Copy link
Member

FineArchs commented Mar 13, 2024

let a=[{a:[]}]
let b=[{a:[]}]
a[0].a[0]=a
b[0].a[0]=b[0]

deepEqual(a,b) // true

になるみたいです

@marihachi marihachi requested review from FineArchs and removed request for syuilo, salano-ym, ikasoba and FineArchs March 14, 2024 13:06
@marihachi
Copy link
Contributor Author

marihachi commented Mar 16, 2024

循環の判定方法にバグがあるかも
両方が循環した位置だけでは一致を判断できないかも?

Comment on lines +12 to +18
// 参照の循環をチェック
// 両方の循環が確認された時点で、その先も一致すると保証できるためtrueで返す
const indexA = refsA.findIndex(x => x === a);
const indexB = refsB.findIndex(x => x === b);
if (indexA !== -1 && indexB !== -1) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

愚直にやるなら、indexAとindexBが同時に見つかるのが二回確認できればその先の一致が保証できると思います(一回だとループの開始位置にズレがある場合があるが、二回なら一回目のループ終了地点で開始を固定できる)

Copy link
Contributor Author

@marihachi marihachi Mar 16, 2024

Choose a reason for hiding this comment

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

aとbで同じ位置で循環が検出される場合は良いと思っています。
aとbでそれぞれ別の位置を起点として循環が現れるが、パターンとしては一致しているor一致してないというのを判定する処理が必要な気がしています。
具体的な処理は思いついていません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ん、2回まわすってことですか?

Copy link
Member

Choose a reason for hiding this comment

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

はい。同位置で循環があった地点からもう一度回せば、「aとbで同じ位置で循環が検出される場合」に持ち込めるのでうまくいくはずです。(だいぶ効率が悪いとは思いますが)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants