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.

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

crypto: use macro map for NodeCryptoError #37758

Conversation

RaisinTen
Copy link
Contributor

No description provided.

@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. needs-ci PRs that need a full CI run. labels Mar 15, 2021
}
errors_.emplace_back(SPrintF(error_string,
std::forward<Args>(args)...));
}
#undef NODE_CRYPTO_ERROR_CODES_MAP
Copy link
Member

Choose a reason for hiding this comment

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

Having this available outside of the header might be useful at some point 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax Agreed, exposed it in 6ce921a. Could you think of any place where it could be used currently? I could incorporate it in a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

No, not right now, although I guess if we add more error codes to CryptoErrorStore::Insert() then it might make sense to move the part that generates error_string/that doesn’t use args back into a .cc file (because right now every call to CryptoErrorStore::Insert() potentially creates a new copy of the function).

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 16, 2021
@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Mar 19, 2021
PR-URL: #37758
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

Landed in 971e009

@jasnell jasnell closed this Mar 19, 2021
@RaisinTen RaisinTen deleted the crypto/use-macro-map-for-NodeCryptoError branch March 20, 2021 16:05
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
PR-URL: #37758
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants