Skip to content
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.

Sign up for GitHub

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

Jump to bottom

crypto: move node_crypto_clienthello-inl.h to cc #17606

Closed
wants to merge 1 commit into from
Closed

crypto: move node_crypto_clienthello-inl.h to cc #17606

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Dec 11, 2017

This commit updates node_crypto.h to just include
node_crypto_clienthello.h instead of the inline header. Also
node_crypto.cc is updated to include node_crypto_clienthello-inl.h.

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

@danbev
crypto: move node_crypto_clienthello-inl.h to cc
445ee97 
This commit updates node_crypto.h to just include
node_crypto_clienthello.h instead of the inline header. Also
node_crypto.cc is updated to include node_crypto_clienthello-inl.h.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Dec 11, 2017
@danbev
Copy link
Contributor Author

danbev commented Dec 11, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/12033/

@BridgeAR
Copy link
Member

BridgeAR commented Dec 12, 2017 •
edited
Loading

Should this be fast tracked?

@BridgeAR
Copy link
Member

Should still be fast tracked?

@danbev
Copy link
Contributor Author

danbev commented Dec 13, 2017

Landed in 47edfd9

@danbev danbev closed this Dec 13, 2017
@danbev danbev deleted the crypto_clienthello-inl-move branch December 13, 2017 08:14
danbev added a commit that referenced this pull request Dec 13, 2017
@danbev
crypto: move node_crypto_clienthello-inl.h to cc
47edfd9 
This commit updates node_crypto.h to just include
node_crypto_clienthello.h instead of the inline header. Also
node_crypto.cc is updated to include node_crypto_clienthello-inl.h.

PR-URL: #17606
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@danbev @MylesBorins
crypto: move node_crypto_clienthello-inl.h to cc
dfb9b5e 
This commit updates node_crypto.h to just include
node_crypto_clienthello.h instead of the inline header. Also
node_crypto.cc is updated to include node_crypto_clienthello-inl.h.

PR-URL: #17606
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

Does this make sense for LTS?

It does not land cleanly on v6.x and creates new compiler warnings for v8.x

../src/node_crypto_clienthello.h:75:15: warning: inline function 'node::crypto::ClientHelloParser::Reset' is not defined [-Wundefined-inline]
  inline void Reset();
              ^
../src/node_crypto_clienthello.h:47:5: note: used here
    Reset();
    ^

@MylesBorins
Copy link
Contributor

ping re: backport

@danbev
Copy link
Contributor Author

danbev commented Feb 27, 2018

@MylesBorins Sorry about the delay, I'll take a look at backporting today.

@danbev
Copy link
Contributor Author

danbev commented Feb 27, 2018

@MylesBorins I don't think this is worth backporting to v6.x as it will require multiple changes to other files in addition to the ones in this pull request.

I'll take a look at the warning backporting to v8.x now.

@danbev
Copy link
Contributor Author

danbev commented Feb 27, 2018

I've taken a look and the same warnings are generated for 8.x as for 6.x. I don't think this is worth back porting. Let me know if you feel differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danbev @BridgeAR @MylesBorins @bnoordhuis @jasnell @nodejs-github-bot