Skip to content

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 14, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 14, 2017
@jasnell jasnell added the http2 Issues or PRs related to the http2 subsystem. label Aug 14, 2017
@jasnell jasnell requested a review from addaleax August 14, 2017 17:11
}
}

#define SESSION_TYPE_NAME(session) SessionTypeName(session->session_type_)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make this a method on Nghttp2Session? That seems easier, plus it avoids needing a #define

(If you want to stick with the define: Parentheses around session ;))

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2017

The functions remaining in node_http2_core can be inlined. Do so
and eliminate the node_http2_core.cc file entirely.
show server name type rather than number for more useful and
clear debug messages
@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2017

jasnell added a commit that referenced this pull request Aug 16, 2017
* inline more stuff. remove a node_http2_core.cc
* clean up debug messages
* simplify options code, and cleanup

PR-URL: #14825
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2017

Landed in 949aec5

@jasnell jasnell closed this Aug 16, 2017
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
* inline more stuff. remove a node_http2_core.cc
* clean up debug messages
* simplify options code, and cleanup

PR-URL: nodejs/node#14825
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
* inline more stuff. remove a node_http2_core.cc
* clean up debug messages
* simplify options code, and cleanup

PR-URL: #14825
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
* inline more stuff. remove a node_http2_core.cc
* clean up debug messages
* simplify options code, and cleanup

PR-URL: #14825
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants