This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
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.
Memory fixes. Resolves #10867, and resolves #14080 #14372
Memory fixes. Resolves #10867, and resolves #14080 #14372
Changes from all commits
f81aec5
1205320
c27efd9
cb2542e
1dee791
5b8d88c
2f3d516
57e5f63
2f84c9d
4576cdc
0a1cbbb
5af91db
33c113c
232d33a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
why do you need this new method?
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.
Yeah so originally I wanted to use the .using method directly but ran into some problems. There are times when we need to add new native resources to the same scope of the parent but there's no guarantee that the parent is actually in an existing scope.
When this happens it leads to a few issues. First, the parent is in None scope which we cannot pass to ResourceScope.using. Second, we still want to execute the body and allocate all the new resources but don't want to want to make a new ResourceScope because all those new resources will disappear with the new scope.
Alternatives that I thought of were: 1.) to have the caller check whether or not it was in a scope and handle it appropriately. This is ugly and puts the onus on the callers in multiple places. 2.) Changing the using method to work with a None scope. I opted not to do this because it complicates that method and I believe would require changing the method parameters which I didn't want to do. 3.) Changing the default scope to be something other than None. This is probably a reasonable solution. Maybe we have some kind of base scope or something similar. That's likely to be a fairly significant change in both the design and behavior of this class.