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

[Justin Lebar via Parallel_libs-commits]

jlebar added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Passkey.h:1
@@ +1,2 @@
+//===-- Passkey.h - Key for method access control ---------------*- C++ -*-===//
+//
----------------
Move into Util?

================
Comment at: streamexecutor/include/streamexecutor/Passkey.h:41
@@ +40,3 @@
+///       // able to create a Passkey<B>, so it is effectively private to B.
+///       void doStuffOnlyBCanDo(Passkey<B> &Key);
+///     };
----------------
Should we take Passkey<B> by value instead of by reference?

================
Comment at: streamexecutor/include/streamexecutor/Passkey.h:55
@@ +54,3 @@
+  /// The pointer used to create the key.
+  T *Reference;
+
----------------
We call it a "reference" in the comment and here, but apparently it's a pointer; that should be harmonized.  I kind of like the idea of keeping a proper C++ reference, so it's clear that it's never null.

================
Comment at: streamexecutor/include/streamexecutor/Passkey.h:62
@@ +61,3 @@
+  /// Allow implicit construction so a T object can pass "this" to a function
+  /// that expects a Passkey<T>& argument.
+  Passkey(T *Reference) : Reference(Reference) {}
----------------
Not sure about the "&".  In fact I would expect that if you have

  void foo(Passkey<Bar>& b);

  and then have

  void Bar::DoSomething() {
    foo(this);
  }

that won't compile because we're trying to bind a temporary Passkey to `b`.

================
Comment at: streamexecutor/include/streamexecutor/Passkey.h:65
@@ +64,3 @@
+
+  /// Disallow copy and assignment construction to prevent cloning.
+  ///@{
----------------
I think you'll need a copy-constructor if you want to pass these by value.  I guess we could take them by rvalue reference if you really wanted to disallow copying.  But even then we'd still need to allow moves, which is almost as bad from an access-control perspective.

So I'm not sure disallowing copying is worthwhile.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:91
@@ +90,3 @@
+      return Error::success();
+  };
+
----------------
> 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.


https://reviews.llvm.org/D23333