-
Notifications
You must be signed in to change notification settings - Fork 90
Added DumpMachine() and DumpRegister(), some refactoring #634
Conversation
|
Please review while I'm resolving the build breaks |
cgranade
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a large PR, so I haven't had the chance to finish reviewing it yet; wanted to give initial feedback on what parts I had looked at so far, though.
swernli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a more full review later, but some of these changes (most notably the ones to the headers in the public folder) are breaking changes, such that I think we should have a more general discussion about which parts of our API boundary have dependencies vs not so we can avoid breaking any consumers.
|
Looks like I missed an important information earlier. I didn't know that our QIR code has customers. |
|
Yeah, it has definitely been the case up until now that we have been more free to do refactoring based on no external dependencies on this API, but that seems to be changing quickly. The changes you are making here are well thought out and reasonable from the point of view of a C++ API, and give us important things to consider. At the same time, we also should start considering ease of interop with other languages, in which case it might be worth leaning toward a more C-style API that relies on extern functions and basic types instead of objects. All these API design concerns are things that only recently have started coming up, so I think it will be important to have your experience and knowledge to take into account with that before we settle on a firm approach, so between that and the few partners who are taking a dependency on the API it seems we are due for a larger team discussion about how we want to handle updates to the design of the API. |
|
I have reverted the breaking changes. Please give another round of review. |
swernli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments for you to consider, but overall changes look good to me (though it does look like there is some ongoing discussion with Chris in a few places).
(Feel free to let me know if you'd like me to break this up into multiple PRs)
Added DumpMachine() and DumpRegister() functions implementation.
Added some README.md files (that I've written for myself, to get a more clear understanding for the future of what to add where, and decided to share those).
Did some refactoring of the code.
Resolves #578.