Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@ScottCarda-MS
Copy link
Contributor

@ScottCarda-MS ScottCarda-MS commented Mar 22, 2020

Adding the basics of the call graph walker.

@ScottCarda-MS ScottCarda-MS mentioned this pull request Mar 24, 2020
@ScottCarda-MS ScottCarda-MS changed the base branch from master to features/CallGraphWalker March 26, 2020 20:44
avasch01
avasch01 previously approved these changes Mar 27, 2020
Copy link

@avasch01 avasch01 left a comment

Choose a reason for hiding this comment

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

:shipit:

@ScottCarda-MS
Copy link
Contributor Author

ScottCarda-MS commented Mar 27, 2020

// Licensed under the MIT License.

Should you remove three unchanged files for this PR?

Refers to: src/QsCompiler/Transformations/Monomorphization.cs:2 in 22c03cd. [](commit_id = 22c03cd, deletion_comment = False)

I had, at various different points, made logic changes to these files, but they were resolved over the course of development. When I change a file, I like to make sure there are no trailing whitespaces left in the file. Those are the remaining changes in these files, and I would like to keep them, as they should not have trailing whitespace.

@avasch01 avasch01 dismissed their stale review March 27, 2020 17:33

revoking review

@ScottCarda-MS ScottCarda-MS requested a review from bamarsha April 2, 2020 21:56
Copy link
Contributor

@bamarsha bamarsha left a comment

Choose a reason for hiding this comment

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

Is it possible to add something that uses the call graph in this PR, or link to example of how it's intended to be used? (Sorry if I'm missing some obvious context here.) Without a concrete example it's hard for me to tell if the design is reasonable for its intended use case. Other than that it looks good.

(Edited: Oh, sorry, I see that #395 uses the call graph to find call cycles.)

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

The one thing missing is the override for partial applications I believe. Please add that before merging. Other than that, lgtm.

@ScottCarda-MS ScottCarda-MS merged commit 6acc255 into features/CallGraphWalker Apr 14, 2020
@bettinaheim bettinaheim deleted the sccarda/CallGraphWalker branch April 26, 2020 21:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants