[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/Passkey.h:1
@@ +1,2 @@
+//===-- Passkey.h - Key for method access control ---------------*- C++ -*-===//
+//
----------------
jlebar wrote:
> Move into Util?
I got rid of this class (see comment below), so I'm marking all the comments on this class as "done". Thanks for suggesting a better way to handle all this.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:91
@@ +90,3 @@
+      return Error::success();
+  };
+
----------------
jlebar wrote:
> > 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.
> 
> Can't SE simply avoid passing out references to `StreamInterface`s, then?
> 
> For the "private-to-Stream" functions in SE, I'm wondering something similar: It seems like maybe SE should be split into two types.  One type is visible to end-users, and it contains only functions they'd call.  The other type is visible only to e.g. Stream, and it actually does work.  Stream would take a pointer to that thing in its constructor.
> 
> Since SE isn't going to give out pointers to this "internal SE" except when creating Streams, we get access control for free.
Yes, I think the design you outline here is much better. I created a class called `StreamParent` to hold the interface that a `Stream` uses to call its parent `StreamExecutor`. I got rid of the `Passkey` class and all the key arguments.

This seems much cleaner. Thanks for the advice!


https://reviews.llvm.org/D23333