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

src,doc: add C++ internals documentation #30552

Closed
wants to merge 13 commits into from
Closed

Conversation

@addaleax
Copy link
Member

addaleax commented Nov 20, 2019

This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

/cc @vsemozhetbyt @jasnell @sam-github

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929
@addaleax addaleax added the C++ label Nov 20, 2019
Copy link
Member

sam-github left a comment

Nice!

src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
Copy link
Member

gengjiawen left a comment

I am strongly inclined to add link to https://github.com/nodejs/node/blob/master/doc/api/addons.md. Also maybe put it docs ?

src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Show resolved Hide resolved
src/README.md Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Show resolved Hide resolved
src/README.md Show resolved Hide resolved
src/README.md Show resolved Hide resolved
addaleax and others added 2 commits Nov 20, 2019
Co-Authored-By: mscdex <mscdex@users.noreply.github.com>
Co-Authored-By: Jiawen Geng <technicalcute@gmail.com>
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 20, 2019

I am strongly inclined to add link to https://github.com/nodejs/node/blob/master/doc/api/addons.md.

The addon docs aren’t good for this, imo. The parts about describing how to set up addons are not relevant here (e.g. NODE_MODULE_... macros) and the examples either don’t apply or don’t reflect best practices.

Also maybe put it docs ?

If you mean moving it to doc/, eh, yeah, I guess we could do that but I like having internals docs close to the source code. (i.e. I’d prefer src/doc/ over something in doc/ but since we don’t have any other C++ docs I’d say src/README.md is fine).

src/README.md Show resolved Hide resolved
@gengjiawen

This comment has been minimized.

Copy link
Member

gengjiawen commented Nov 20, 2019

The addon docs aren’t good for this, imo. The parts about describing how to set up addons are not relevant here (e.g. NODE_MODULE_... macros) and the examples either don’t apply or don’t reflect best practices.

Addon can use v8 api, node.h things like that. I think current doc lack this kind of content. This will give developers many help if we include this.

If you mean moving it to doc/, eh, yeah, I guess we could do that but I like having internals docs close to the source code.

To be fare, they are still in the same repo if we put it in doc. I do think this kind of content need more exposure. It can be very helpful and valuable to whole community.

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 20, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 20, 2019

The addon docs aren’t good for this, imo. The parts about describing how to set up addons are not relevant here (e.g. NODE_MODULE_... macros) and the examples either don’t apply or don’t reflect best practices.

Addon can use v8 api, node.h things like that. I think current doc lack this kind of content. This will give developers many help if we include this.

Yeah, and if we had good addon docs, that would be massively helpful and I’d totally be in favour of linking them- But imo they’re not helpful in their current state which is kind of okay for me given that we’ve been pushing N-API for a while.

If you mean moving it to doc/, eh, yeah, I guess we could do that but I like having internals docs close to the source code.

To be fare, they are still in the same repo if we put it in doc. I do think this kind of content need more exposure. It can be very helpful and valuable to whole community.

I mean, these are docs about internals specifically. I sure hope that they are helpful and valuable to people who work on files in src/, and so having the visibility for those people has the highest priority for me.

Copy link
Member

joyeecheung left a comment

I have some comments, but nothing blocking. Thanks for working on this!

src/README.md Outdated Show resolved Hide resolved
src/README.md Show resolved Hide resolved

There is no way to abort libuv requests in general. If a libuv request is not
managed through a [`ReqWrap`][] instance, the
`env->IncreaseWaitingRequestCounter()` and

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 20, 2019

Member

I am slightly concerned direct references to these methods could get out of date. Can't we just write these down in env.h and link to the headers instead?

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 20, 2019

Author Member

Can't we just write these down in env.h and link to the headers instead?

I’m not sure what you mean? But either way, I think keeping this doc up to date and making sure of that during reviews is something we’ll have to do either way.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 20, 2019

Member

I think keeping this doc up to date and making sure of that during reviews is something we’ll have to do either way.

I think if keeping this doc up-to-date is going to be a blocker for landing changes in src/, we should just move the information about the internal classes into the headers as comments, and link to those in the README. For one if the README is too long it becomes difficult to read, also it's going to be a pain to remember updating this doc while changing the code - but slightly less so if the comments live in the headers.

Also it feels weird to me that in order to understand some of these classes at a higher level, you either have to read the code, or read this doc because what's supposed to be said in comments are moved into a markdown file...I think it would be more helpful if the README is just a table of contents.

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Nov 20, 2019

Member

tbh, I really like the proposed format. It has a flow, the necessary level of abstraction, example code at right places, and caveats and cautions as appropriate - something either not possible in code comments, or not easily consumable. IMO inline comments are rich with the context, but weak in connecting bigger views, and more importantly, discoverability.

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 20, 2019

Author Member

If you have items that you feel should also be documented more explicitly in a header file, I’m happy to do that, but generally I agree with @gireeshpunathil – this is not supposed to be an exhaustive guide or reference documentation, but rather paint the big picture and distinguish what’s important from what’s not.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 20, 2019

Member

and more importantly, discoverability.

I feel the opposite - I think content in the README is more difficult to discover than content in the headers, especially when the README becomes long. For me it's rare that I'll read a doc, top to bottom, to understand a code base and remember all these identifier names. I usually run into these names when I hack on the code and I expect to see explanations in the headers.

this is not supposed to be an exhaustive guide or reference documentation, but rather paint the big picture and distinguish what’s important from what’s not.

That's what I think this document should be like as well, but with details like

They can be added and removed through by using env->AddCleanupHook(callback, hint); and env->RemoveCleanupHook(callback, hint);, where callback takes a void* hint argument.

and

If a libuv request is not managed through a [ReqWrap][] instance, the env->IncreaseWaitingRequestCounter() and env->DecreaseWaitingRequestCounter() functions need to be used to keep track of the number of active libuv requests.

It does not seem to be the case. These references to specific method names and their arguments do not seem to be necessary in terms of giving the reader a bigger view of the code base.

This comment has been minimized.

Copy link
@addaleax

addaleax Nov 20, 2019

Author Member

and more importantly, discoverability.

I feel the opposite - I think content in the README is more difficult to discover than content in the headers, especially when the README becomes long.

That’s somewhat solvable by adding a table of contents, if you like?

this is not supposed to be an exhaustive guide or reference documentation, but rather paint the big picture and distinguish what’s important from what’s not.

That's what I think this document should be like as well, but with details like

They can be added and removed through by using env->AddCleanupHook(callback, hint); and env->RemoveCleanupHook(callback, hint);, where callback takes a void* hint argument.

and

If a libuv request is not managed through a [ReqWrap][] instance, the env->IncreaseWaitingRequestCounter() and env->DecreaseWaitingRequestCounter() functions need to be used to keep track of the number of active libuv requests.

It does not seem to be the case. These references to specific method names and their arguments do not seem to be necessary in terms of giving the reader a bigger view of the code base.

Well … where would you document these, especially the latter one? It’s easy to add inline docs to ReqWrap but what do you do when you want to describe when it’s about not using ReqWrap?

In any case, I don’t feel like these give away too much detail. Judging from the current contents of the file in this PR, the number of yearly PRs that would require updating it is probably in the single digits.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 20, 2019

Member

That’s somewhat solvable by adding a table of contents, if you like?

I think that does not help in this case. To quote my edit:

For me it's rare that I'll read a doc, top to bottom, to understand a code base and remember all these identifier names. I usually run into these names when I hack on the code and I expect to see explanations in the headers.

Now this is solvable if in the headers we point the readers to the README for more information, but that feels kind of weird.

where would you document these, especially the latter one?

I think if the goal is to give a big picture in the README, for the latter one it can just say something like:

If a libuv request is not managed through a [ReqWrap][] instance, a counter is used to make sure it gets cleaned up.

And the method names can be documented in the req_wrap.h or env.h. The README can just point the readers to that header to see these details. This is a README for src/ after all, so I'd expect it to be more of a table of contents for all these files under src/.

addaleax added 2 commits Nov 20, 2019
@addaleax addaleax force-pushed the addaleax:cpp-docs branch from 4c89674 to e1ca24c Nov 20, 2019
src/README.md Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/README.md Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
addaleax and others added 2 commits Nov 20, 2019
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 20, 2019

@cjihrig Done, and added documentation for internal fields; PTAL

src/README.md Outdated Show resolved Hide resolved
@gireeshpunathil

This comment has been minimized.

Copy link
Member

lundibundi left a comment

LGTM, great work.

src/README.md Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
Copy link
Member

jasnell left a comment

WOO! 🎉 💯 🥇

addaleax and others added 2 commits Nov 22, 2019
Co-Authored-By: Denys Otrishko <9109612+lundibundi@users.noreply.github.com>
Copy link
Member

mhdawson left a comment

LGTM, @addaleax thanks for putting it together. Great resource for new contributors.

addaleax added a commit that referenced this pull request Nov 22, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 22, 2019

Landed in c132035, thanks for the reviews!

@addaleax addaleax closed this Nov 22, 2019
@addaleax addaleax deleted the addaleax:cpp-docs branch Nov 22, 2019
[`async_wrap.h`]: async_wrap.h
[`base_object.h`]: base_object.h
[`handle_wrap.h`]: handle_wrap.h
[`memory_retainer.h`]: memory_retainer.h

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Nov 24, 2019

Contributor

Sorry to be late. This link seems 404 now.

[cleanup hooks]: #cleanup-hooks
[event loop]: #event-loop
[exception handling]: #exception-handling
[internal field]: #internal-field

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Nov 24, 2019

Contributor
- [internal field]: #internal-field
+ [internal field]: #internal-fields
data if it is typically expected to be small (e.g. file paths).

The `Utf8Value`, `TwoByteValue` (i.e. UTF-16 value) and `BufferValue`
(`Utf8Value` but copy data from a `Buffer` is that is passed) helpers

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Nov 24, 2019

Contributor

Is there a typo here?

addaleax added a commit to addaleax/node that referenced this pull request Nov 28, 2019
@addaleax addaleax mentioned this pull request Nov 28, 2019
3 of 3 tasks complete
@sam-github sam-github mentioned this pull request Nov 28, 2019
0 of 4 tasks complete
addaleax added a commit that referenced this pull request Nov 30, 2019
Refs: #30552 (review)

PR-URL: #30693
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax added a commit that referenced this pull request Nov 30, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax added a commit that referenced this pull request Nov 30, 2019
Refs: #30552 (review)

PR-URL: #30693
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos added a commit that referenced this pull request Dec 1, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
targos added a commit that referenced this pull request Dec 5, 2019
Refs: #30552 (review)

PR-URL: #30693
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
Refs: nodejs#30552 (review)

PR-URL: nodejs#30693
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
Refs: #30552 (review)

PR-URL: #30693
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.