[Parallel_libs-commits] [PATCH] D24063: [StreamExecutor] Add Stream::blockHostUntilDone

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:87
@@ +86,3 @@
+  // Returns the result of getStatus() after the Stream work completes.
+  Error blockHostUntilDone() {
+    setError(PDevice->blockHostUntilDone(ThePlatformStream.get()));
----------------
jlebar wrote:
> jhen wrote:
> > jlebar wrote:
> > > Would "thenBlockHostUntilDone" be more consistent with the other names?  Or are you intentionally being inconsistent to signal that this is a different sort of thing (e.g. it doesn't return a Stream&)?
> > Right, I'm being intentionally inconsistent for just the reason you gave.
> > 
> > Side note: things are kind of funny now because there is no `Stream::init` method. It used to be `S->init().thenA().thenB().block()`, now it's `S->thenA()->thenB().block()`. I almost want to change the names so it's `S->aThen().bThen().block()`, but of course that doesn't read well because the parameter list is after the "then".
> I was wondering about that...
> 
> Of course even before you could do something like
> 
>   Stream *s = ...;
>   s->init()->thenA().thenB().block();
>   s->thenC().thenD().block();
> 
> Right?  That is, we only init the stream once.  But I guess that's a less-common use-case?
> 
> Should we encourage people to treat streams as ephemeral?
> 
>   Device.getStream()->thenA().thenB().thenC();
> 
> That doesn't scan so bad.
> 
> While we're thinking about it, it's also weird that you have to switch between `.` and `->`.  Although I'm not sure that returning a `Stream*` would be an improvement.  Maybe...
You're right, it was a problem even with the `init` function because the stream could be reused.

It's probably not right to suggest that `Stream` references shouldn't be held. The most common case is probably to create one `Stream` and then use it for everything in your program, and there is also overhead in creating and destroying streams.

We could be silly and make an extra "doer" object that would just be there to make things read like English:

```
Doer Stream::listenUp();
Doer &Doer::thenA();
Doer &Doer::thenB();
...
Stream *s = ...;
s->listenUp().thenA().thenB();
s->block();
```
:) We could make all `Doer` instances ephemeral. I better stop talking about this, I think I'm actually talking myself into this joke suggestion.

I'm also not thrilled with the switch from `->` to `.`, but I think I'd rather live with it than use all `->`.


Repository:
  rL LLVM

https://reviews.llvm.org/D24063