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 errors in WhyDgml and account for cycles #106928

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

Conversation

jtschuster
Copy link
Member

Fixes compiler errors and adds a "visited" parameter to account for cycles in the dependency graph.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

The comment on lines 14-15 might need updating if we really want to go with this.

I assume hitting cycles would be pretty common in regular DGML files. I don't know how useful this extra cycle handling is. Sure, now the program will terminate, but answers with (cycle) appended to it are not be very helpful (they don't end up telling the path to the root, which is the entire purpose of the whydgml tool).

One would either need a more sophisticated graph algorithm turning this short tool into something complex, or... just implementing the FirstMarkLogStrategy in ILLinker's DGML writer. My preference would be on the later.

(I don't mind either way, I'm not going to use this with graphs that have cycles.)

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

Successfully merging this pull request may close these issues.

3 participants