Update Trieste and fix API breakage #161
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note: this branch's build will probably break because of an issue in Trieste's new tests that only shows up in dependencies. |
|
Note: microsoft/Trieste#129 tracks the upstream considerations we should resolve or defer before considering this. |
fhackett
force-pushed
the
fhackett-update-trieste
branch
from
0da45bb
to
dc70b24
Compare
|
Build breakages aside, the actual code changes are ready for review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After Trieste's recent intrusive refcount change, uses of
->shared_from_this()broke.This PR shows the changes needed for fix that.
For broader discussion (don't merge yet!):seems my change to Trieste added tests in a way that breaks in dependencies. Oops, will fix.As part of intrusive_ptr discussion, there were thoughts of making->parent()return aNodeinstead of aNodeDef*All high-level issues resolved. The rest is a matter of discussing the adaptations I made to
->parent(), which mostly involve shortening->parent()->shared_from_this(), though it is not done entirely robotically and there might be a style judgement call here or there.