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.
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
Don't create the default allocator every single time. Rename API accordingly. Expose Session/Run log severity levels. #1615
Don't create the default allocator every single time. Rename API accordingly. Expose Session/Run log severity levels. #1615
Changes from 15 commits
b03dc6f
cc5b316
4ef6284
85bd0dc
42aa76c
27ac7f1
8fdbd2e
704934b
10f46b4
dcd6982
723ded7
a2c7ae3
d07e8ff
fe09f6d
1f0a904
003fbd4
06c0af6
12e2034
0473368
b999b82
58863c4
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.
Had to change this because Base relies on the existence of OrtRelease function which we don't want to support for this statically allocated allocator.
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.
Is there any other use for this class besides holding the Allocator with default options?
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.
But this change will affect all the alloactors, right? Not just the default one.
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.
Currently this seems to be used only for allocators with default options. One idea is to rename this class to AllocatorWithDefaultOptions. That way if we've a different use, we can have a generic Allocator class that (derives from Base) and hence supports creation and deletion every single time. Would renaming it to AllocatorWithDefaultOptions make it more clear?
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.
@RyanUnderhill and I had an offline conversation and we think that using a new name AllocatorWithDefaultOptions + creating it in the constructor might be the lesser of the evils for this. Updated code.