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

fix wrong cycle detect by clone the map[visit]int #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alingse
Copy link

@alingse alingse commented Apr 20, 2023

use visited map[visit]int to check cycle might get some bad case.

the key reason was that, all the pp, p printer share a same map[visit]int

but when we process a struct refer a pointer value twice or more, this is not a cycle, it's just a repeat refer.

maybe use other helper data type for every branch when we call code pp = *p statement

maybe a slice is enough to detect.

branch 1 : [v1, v2, v3, v5]
branch 2 : [v1, v2, v3, v4]
branch 3 : [v1, v2, v3, v4, v1]

only branch3 should print a "CYCLIC REFERENCE"

@alingse alingse changed the title try fix wrong detect on cycle Draft: try fix wrong detect on cycle Apr 20, 2023
@alingse
Copy link
Author

alingse commented Apr 20, 2023

this is only a draft PR, and i will code more to try fix this. cc @kr

btw, thanks for this great repo.

@kr
Copy link
Owner

kr commented Apr 21, 2023

Good catch, thank you! I just opened #96 for the bug itself. That should automatically be closed when this PR gets merged.

@kr kr linked an issue Apr 21, 2023 that may be closed by this pull request
@alingse alingse force-pushed the fix-visit-map-reuse branch from 1972968 to b801321 Compare April 21, 2023 13:14
@alingse alingse force-pushed the fix-visit-map-reuse branch from b801321 to e042962 Compare April 21, 2023 13:16
@alingse alingse changed the title Draft: try fix wrong detect on cycle fix wrong cycle detect by clone the map[visit]int Apr 21, 2023
@alingse
Copy link
Author

alingse commented Apr 21, 2023

cc @kr I have add a clone func to copy the visited map, and add some test about cycle
I test in local go test ./... it successed, need your review and comment if you have some.

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.

false positive in cycle check
2 participants