[Parallel_libs-commits] [PATCH] D23333: [StreamExecutor] Add basic Stream operations

[Jason Henline via Parallel_libs-commits]

jhen added a comment.

In https://reviews.llvm.org/D23333#511039, @jprice wrote:

> I think I understand the motivation for restricting the use of `StreamExecutor::launch/memcpy` etc to only the `Stream` class via the `StreamKey` struct, but I'm not sure I see the need for the `StreamExecutorKey` struct. It seems like this is currently just protecting the `Stream` constructor, but is it really a problem if someone can write `new Stream(Executor, nullptr)` as well as `Executor->createStream()`? Don't have strong opinions about this, just curious.


My goal was to make sure that Streams always have non-null implementation pointers. Basically, I didn't want any half-formed Stream objects floating around. This goal was really undercut when I originally left in the code to allow Streams to be moved. After being moved from, a Stream would be half-formed. But now I've removed the move stuff for Streams, and I think that leaves us with a useful invariant that Streams will always be fully-formed.

I added a comment to the Stream constructor to try to clarify this because I think you're right that it's a bit confusing, and I think others will have the same question.

> The `StreamExecutor` class seems to be missing a virtual destructor (warned by Clang).


Thanks for catching that. I have added in a virtual destructor now.


https://reviews.llvm.org/D23333