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

fixed incorrect callbacks to finish_edge in undirected_dfs #390

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

danielyxyang
Copy link
Contributor

Resolves #389

@jeremy-murphy
Copy link
Contributor

Not sure why all those Drone checks failed, but you probably should update to the tip of develop anyway because I have fixed some CI configuration.

@jeremy-murphy
Copy link
Contributor

Please add a unit test that would fail on develop as it is but passes with your proposed change.

stack.push_back(std::make_pair(u,
std::make_pair(src_e, std::make_pair(++ei, ei_end))));
std::make_pair(src_e, std::make_pair(ei+1, ei_end))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably has to be this so that it works with edge iterators that aren't random access.

Suggested change
std::make_pair(src_e, std::make_pair(ei+1, ei_end))));
std::make_pair(src_e, std::make_pair(std::next(ei), ei_end))));

And, call me fussy, but I prefer this style anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, makes sense! I'm not very fluent in C++, so thanks for pointing this out :)

@danielyxyang
Copy link
Contributor Author

I added a unit test now based on the minimal working example in #389. I tested the test hehe and it passes with the changes I made and fails without these changes.

Disclaimer: I know my coding style is quite different from what mostly prevails in this codebase. So feel free to adapt anything you think is important for maintaining consistency. Similarly, as C++ is not my main coding language, I'm sure there are quite a few things someone with more experience would have done better or differently. So feel free to make changes in this regard too :) Not sure though how the workflow here on Github is and whether you need write access to my fork to be able to make edits...

@jeremy-murphy
Copy link
Contributor

jeremy-murphy commented Oct 24, 2024

Huh, that's weird, the GitHub actions CI doesn't seem to be running, and the Drone CI ran instantly. :|
Sorry, I'll have to investigate what's going on with the CI before I can merge any PRs.
PS. Oh, weird, GitHub said that "all checks passed" and then later started the GitHub actions CI.

@jeremy-murphy jeremy-murphy merged commit 4d6a196 into boostorg:develop Oct 30, 2024
6 checks passed
@jeremy-murphy
Copy link
Contributor

Thanks again!

@jeremy-murphy
Copy link
Contributor

I was happy with the implementation, so I merged it, but actually the test could do with some improvement if you have time.
Using a string as the output type works, but it's a little fragile. What would be better is a scoped enum, but since the output could be a vertex or an edge, it's not simple how to represent it. I would lean towards a sum type, but I'm open to alternative suggestions. Feel free to discuss ideas before you implement, if you're willing to take it on.
I was also trying to think of more tests to add, and so far the only slightly meaningful test case I can think of is a V-shaped graph with three vertices. If the search starts from the obvious root, then the test will confirm that one branch is finished before the other one is started. It's not really related to the original bug anymore, it's just an edge case that I thought is worth having a test for.

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.

Bug in undirected_dfs causing incorrect callbacks to finish_edge of dfs_visitor
2 participants