Skip to content

src: remove unused _external getters #13535

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 7, 2017

These getters are unused in our source code, untested (apart from a
test checking that the getters themselves do not throw) and are
useless for JS code due to the opaqueness of v8::Externals.

I can’t really imagine addons depending on this, especially as it’s
undocumented and very niche anyway, but I guess there’s no reason
to not consider this semver-major.

@indutny You introduced this in 6e08bb9, do you happen to remember why?

Ref: #13503

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

src/crypto

@addaleax addaleax added crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 7, 2017
@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 Jun 7, 2017
These getters are unused in our source code, untested (apart from a
test checking that the getters themselves do not throw) and are
useless for JS code due to the opaqueness of `v8::External`s.

I can’t really imagine addons depending on this, especially as it’s
undocumented and very niche anyway, but I guess there’s no reason
to not consider this semver-major.

Ref: nodejs#13503
@addaleax addaleax force-pushed the crypto-drop-externals branch from 504e04e to 5211152 Compare June 7, 2017 22:21
@indutny
Copy link
Member

indutny commented Jun 7, 2017

I introduced them for a use-case in C++ addon that I had at that time. I can't recall what exactly that addon did, but it is practically impossible to fiddle with SSL and SSL_CTX pointers in any other way.

Does having them in core really harm anyone? Should we just document them?

@addaleax
Copy link
Member Author

addaleax commented Jun 7, 2017

@indutny Yes, if you think these make sense to keep around I’d really prefer to document them. Otherwise they end up in this weird space between public API and internals, where nobody can even tell whether it’s a supported API or not.

@mscdex
Copy link
Contributor

mscdex commented Jun 7, 2017

If they're going to be documented they shouldn't be underscore-prefixed.

@refack
Copy link
Contributor

refack commented Jun 7, 2017

From a quick look it seems the JS Accessor is just for testing (pre cctest days maybe)?
Will removing the accessors and test with cctest be a reasonable solution?

@addaleax
Copy link
Member Author

addaleax commented Jun 7, 2017

@refack No, @indutny just explained what they were introduced for. There is also no test for their functionality, so I really don’t know where you’re getting that from.

@refack
Copy link
Contributor

refack commented Jun 7, 2017

From I introduced them for a use-case in C++ addon, but I'm probably jumping to conclusions.

@addaleax
Copy link
Member Author

addaleax commented Jun 7, 2017

@refack We could probably get an addon test for these together, but I agree with what the others have said here; the current way in which they are exposed is not really meaningful.

@refack
Copy link
Contributor

refack commented Jun 7, 2017

@refack We could probably get an addon test for these together, but I agree with what the others have said here; the current way in which they are exposed is not really meaningful.

Sounds like an interesting venture. I haven't written a native addon since v0.10 👴

@addaleax addaleax closed this Jul 23, 2017
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++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants