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

Methods of wrapper classes should be marked const #870

Closed
seishun opened this issue Dec 30, 2020 · 3 comments
Closed

Methods of wrapper classes should be marked const #870

seishun opened this issue Dec 30, 2020 · 3 comments
Labels

Comments

@seishun
Copy link
Contributor

seishun commented Dec 30, 2020

For example, Object::Set and ThreadSafeFunction::Release. They don't modify the object itself but only call functions on the N-API object they wrap. This is similar to how the constness of a pointer is orthogonal to the constness of the object it points to.

These methods being non-const means that if I have to call them in a lambda, I have to either capture the object by reference (unidiomatic and not always possible, e.g. with TheadSafeFunction) or mark the lambda as mutable (unusual and misleading since the lambda itself doesn't mutate).

@mhdawson
Copy link
Member

mhdawson commented Jan 4, 2021

@seishun is adding const going to break any existing code (I'm expecting not) and any interest in submitting a PR ?

@seishun
Copy link
Contributor Author

seishun commented Jan 5, 2021

is adding const going to break any existing code (I'm expecting not)

Object::operator[] returns a different type depending on constness (Value or PropertyLValue), but PropertyLValue is implicitly convertible to Value, so I guess it's fine? 🤔

any interest in submitting a PR

Sure, working on it.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2021

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions github-actions bot added the stale label Apr 6, 2021
@github-actions github-actions bot closed this as completed May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants