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

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:90
@@ +89,3 @@
+  // This protection prevents a user from creating a Stream with a nullptr
+  // implementation.
+  Stream(StreamExecutorKey SK, std::unique_ptr<StreamInterface> Implementation);
----------------
jlebar wrote:
> If that's the only point, could we just assert that Implementation is not null?
> 
> One advantage of getting rid of SEKey would be, that (maybe?) we could move StreamKey into Stream itself.  Right now we can't do that because you can't forward-declare a nested type, and so there's no way to have StreamKey inside Stream and SEKey inside SE and have them both depend on each other via #includes.
Thinking more about this, I realized that the more important point is that I don't want users dealing with the `StreamInterface` class at all. It should be private to `StreamExecutor`. So I think it is good to mark this interface as only callable by `StreamExecutor` so user's don't get the wrong idea about `StreamInterface`. I updated the constructor comment to say this instead of talking about null pointers.

I agree that I don't like the way the `StreamKey` is part of the `StreamExecutor` class and vice versa. To deal with this, I factored the whole key object pattern out into a header called `Passkey<T>` that makes it possible for any class to make its own key instance. I think this cleans up the design significantly.


https://reviews.llvm.org/D23333