-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: refactor callback #defines into C++ templates #18133
Conversation
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take.
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.
LGTM as long as CI passes (I'm assuming there's no weird cases here)
src/node_http_parser.cc
Outdated
|
||
// These are helper functions for filling `http_parser_settings`, which turn | ||
// a member function of Parser into a C-style HTTP parser callback. | ||
template <int (Parser::* Member)()> |
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.
It's possible to make the template more generic and avoid code duplication like this:
template <typename Parser, Parser> struct Proxy;
template <typename Parser, typename ...Args, int (Parser::*Member)(Args...)>
struct Proxy<int (Parser::*)(Args...), Member> {
static int Raw(http_parser* p, Args ... args) {
Parser* parser = ContainerOf(&Parser::parser_, p);
return (parser->*Member)(std::forward<Args>(args)...);
}
};
typedef int(Parser::*Call)();
typedef int(Parser::*DataCall)(const char* at, size_t length);
const struct http_parser_settings Parser::settings = {
Proxy<Call, &Parser::on_message_begin>::Raw,
Proxy<DataCall, &Parser::on_url>::Raw,
...
}
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.
Wow! I tried going into that direction but couldn’t get it to compile 😄 Would you prefer your approach? I feel like the current version is already a lot to digest, but your approach is kind of cool…
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 personally prefer DRYing code even it involves a bit more template magic (I mean, that's the point of using templates right XD)..but it's totally fine that you keep it this way.
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.
@joyeecheung Okay, done! Kinda still can’t believe it works. 😄
Landed in 6283077 |
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: #18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: #18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: #18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Should this be backported to |
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: nodejs#18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: #18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: #18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Use template helpers instead of `#define`s to generate the raw C callbacks that are passed to the HTTP parser library. A nice effect of this is that it is more obvious what parameters the `Parser` methods take. PR-URL: #18133 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Use template helpers instead of
#define
s to generatethe raw C callbacks that are passed to the HTTP parser library.
A nice effect of this is that it is more obvious what
parameters the
Parser
methods take.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
src/node_http_parser.cc