Conversation
Mostly this means avoiding including stdlib headers and kj/debug.h if possible.
harrishancock
requested review from
a-robinson,
bretthoerner,
byule,
jasnell,
jclee,
kentonv and
mikea
as code owners
src/workerd/util/co-capture.h
Outdated
|
|
||
| template<typename ...Args> | ||
| static auto coInvoke(Functor functor, Args&&... args) -> ReturnType<Args&&...> { | ||
| static auto coInvoke(Functor functor, Args&&... args) |
There was a problem hiding this comment.
the name functor here confuses me a lot.
As far as I remember the theory, in expression map(list, f) - map is a functor. f is a function.
Or more exactly: map(*, f) is a list->list functor. (https://en.wikipedia.org/wiki/Functor_(functional_programming)#:~:text=In%20functional%20programming%2C%20a%20functor,in%20Haskell%20using%20type%20class)
So in this context CaptureForCoroutine is a functor. And it operates on functions.
Wdyt?
There was a problem hiding this comment.
I agree with you, but the misnomer is already quite prevalent in this codebase. I refrained from changing the names (I prefer "function object", or maybe nowadays "invokable", myself) to avoid opening the can of worms. :)
There was a problem hiding this comment.
It is a misnomer, but it's a broader C++ community misnomer, not one unique to this code. That wiki page itself hints at why it ended up getting used here:
In C++, the name functor is commonly used to refer to a function object, even though the ISO/IEC 14882 standard specification itself exclusively uses the latter term.
I don't think anyone would be too upset if you just renamed this to fn though.
src/workerd/util/co-capture.h
Outdated
| maybeFunctor, "Attempted to invoke CaptureForCoroutine functor multiple times")); | ||
| KJ_IREQUIRE(maybeFunctor != nullptr, | ||
| "Attempted to invoke CaptureForCoroutine functor multiple times"); | ||
| auto localFunctor = kj::mv(*kj::_::readMaybe(maybeFunctor)); |
There was a problem hiding this comment.
why not KJ_IF_MAYBE? Can't you write the same code using it?
There was a problem hiding this comment.
It's just a microoptimization that the compiler could also probably figure out. If I used a KJ_IF_MAYBE, I would be adding a redundant nullness check.
|
Hmm did GitHub automatically add all codeowners as reviewers? That's a little annoying, I'd prefer that people still choose a specific reviewer. |
Yup. |
|
I think you missed an include here: https://github.com/cloudflare/workerd/blob/main/src/workerd/io/worker.c%2B%2B#L18 |
Came to report this too src/workerd/io/worker.c++:18:10: fatal error: 'workerd/util/co-capture.h' file not found
#include <workerd/util/co-capture.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated. |
|
D'oh! Thanks for the reports, fix up here: #84 |
There is a feature request for this here: https://github.com/orgs/community/discussions/35673. |
6 participants
It bugged me that
coCapture()didn't live in libkj -- it's something that pretty much everyone who uses coroutines will need.So far this branch only applies a couple cleanups tocoCapture()to make it more palatable to libkj. I then copied the result verbatim to this counterpart capnproto PR: capnproto/capnproto#1561. Once that is merged, I'll add another commit to update workerd's capnproto dependency andkj::-qualify any uses of the function.