Address follow-up comments on multistream pr #13495#13992
Conversation
|
Can you please change the title and desc of this PR to say that you're addressing follow-up comments? In reply to: 1354076579 In reply to: 1354076579 In reply to: 1354076579 |
|
nodiscard is in C++17. Actually, you can replace all ORT_MUST_USE_RESULT with [[nodiscard]] to make the code more readable. In reply to: 1354336784 In reply to: 1354336784 In reply to: 1354336784 |
| << stats_.DebugString(); | ||
| } | ||
|
|
||
| #ifdef ENABLE_STREAM |
There was a problem hiding this comment.
It's better to add a prefix to such names to avoid conflicts. You added this macro def to all external libraries, for example protobuf, which is not needed. #Resolved
There was a problem hiding this comment.
Yeah, this should be renamed to something like ORT_ENABLE_STREAM
There was a problem hiding this comment.
Thanks for the suggestion. Let me talk to Cheng about this name change once he is back
jslhcl
changed the title
|
Changed as you suggested In reply to: 1354076579 |
|
Thanks. I will leave it as unchanged In reply to: 1354336784 |
| max_dead_bytes_per_chunk, | ||
| initial_growth_chunk_size_bytes)); | ||
| #else | ||
| ORT_THROW("StreamAwareArena should be transparent to minimal build."); |
There was a problem hiding this comment.
But minimal build typical doesn't have exceptions. So, here it will simply crash? #Resolved
There was a problem hiding this comment.
A minimal build can optionally disable exceptions (but typically if you're wanting the minimum size you'd do that).
The ORT_THROW macro in that build will log the error and abort, which is all it can do.
There was a problem hiding this comment.
Thanks for clarification. Then I just close this comment
| using WaitNotificationFn = std::function<void(Stream&, synchronize::Notification&)>; | ||
| void* AllocateBufferWithOptions(std::shared_ptr<IAllocator>& allocator, size_t size, bool use_reserve, Stream* stream, WaitNotificationFn wait_fn); | ||
| void* AllocateBufferWithOptions(IAllocator* allocator, size_t size, bool use_reserve, Stream* stream, WaitNotificationFn wait_fn); | ||
|
|
There was a problem hiding this comment.
nit: IAllocator& so the API communicates it requires the value to be non-null (at least the current implementation does). #Resolved
I was actually asking from them all to change to use nodiscard to a) be consistent and b) use a built-in language feature in preference to our macro. In reply to: 1356930685 In reply to: 1356930685 |
|
Sorry for the misunderstanding. Replaced ORT_MUST_USE_RESULT with nodiscard In reply to: 1356930685 |
5 participants
Description
This PR is to address follow-up comments for the multi-stream pr #13495
Changes including:
Motivation and Context
This PR is to address follow-up comments for the multi-stream pr #13495