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

[Justin Lebar via Parallel_libs-commits]

jlebar added a comment.

Sorry to keep nit'ing on the design here.  I think each revision has been an improvement, though; hopefully we don't start going in circles.


================
Comment at: streamexecutor/include/streamexecutor/PlatformInterfaces.h:42
@@ +41,3 @@
+/// Platform-specific stream handle.
+class PlatformStream {
+public:
----------------
If Stream is still calling into PlatformSE to do its work (so this class is just an RAII thing), maybe PlatformStreamHandle would be a better name?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:64
@@ +63,3 @@
+  Stream(PlatformStreamExecutor *PlatformExecutor,
+         std::unique_ptr<PlatformStream> PStream);
+
----------------
Now we're kind of taking redundant information here.  PStream's PE must match PlatformExecutor, right?

It doesn't seem so bad to me if PStream exposed a pointer back to its PExecutor...

================
Comment at: streamexecutor/include/streamexecutor/StreamExecutor.h:30
@@ -26,1 +29,3 @@
 public:
+  StreamExecutor();
+  StreamExecutor(PlatformStreamExecutor *PlatformExecutor);
----------------
What does it mean to have an SE without a backing PlatformSE?

================
Comment at: streamexecutor/include/streamexecutor/StreamExecutor.h:31
@@ +30,3 @@
+  StreamExecutor();
+  StreamExecutor(PlatformStreamExecutor *PlatformExecutor);
+  virtual ~StreamExecutor();
----------------
Presumably you want explicit?


https://reviews.llvm.org/D23333