-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dead code: legacy read/write ops #2776
Conversation
@ry in |
@bartlomieju thanks - fixed. |
636f45f
to
e2ec9b7
Compare
@@ -72,21 +69,23 @@ fn test_parse_min_record() { | |||
assert_eq!(parse_min_record(&buf), None); | |||
} | |||
|
|||
pub fn dispatch_minimal( | |||
pub type MinimalOp = dyn Future<Item = i32, Error = ErrBox> + Send; | |||
pub type Dispatcher = fn(i32, Option<PinnedBuf>) -> Box<MinimalOp>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this name. I always imagined "dispatcher" as a function which maps "op id" to "op handler".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what this does (modulo the op id part)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmm, I'm not sure. It looks like Dispatcher
is actually an "op", it returns MinimalOp
which is the result of that op. I had a feeling that it was supposed to be that Dispatcher
selects MinimalOp
which returns some MinimalOpResult
. But that's nomenclature... just thinking out loud
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Op = Promise = Future .. there are many ops for each dispatcher. An OpDispatcher (yet to be defined but outlined in #2730) is something that returns an Op.
So I think the term Dispatcher is appropriate here. That said, I consider this bit ephemeral as I plan on adding the OpDispatcher trait soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this definitely makes some hackyness a cleaner, and 'flatbuffer free' is going to become the norm not the exception.
Seem to have caused master to go red. Cause unknown currently unknown, see denoland#2787. This reverts commit 498f6ad.
No description provided.