Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783843002:
[api] Improve documentation for API handle types. (Closed)
|
Created:
3 years, 8 months ago by Anna Henningsen Modified:
3 years, 8 months ago Reviewers:
Franzi CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[api] Improve documentation for API handle types.
Add a few explanations to the documentation several methods and classes,
in particular Local, MaybeLocal, the HandleScopes.
Drive-by-fix: turn a few regular comments into documentation comments.
BUG=
Review-Url: https://codereview.chromium.org/2783843002
Cr-Commit-Position: refs/heads/master@{#44243}
Committed: https://chromium.googlesource.com/v8/v8/+/a63744d50d0e8ee64446acc042b07708e69ec2ab
Patch Set 1 #
Total comments: 16
Patch Set 2 : Improve documentation comments in v8.h #Patch Set 3 : [api] Improve documentation for API handle types #Patch Set 4 : [api] Improve documentation for API handle types. #Messages
Total messages: 18 (11 generated)
addaleax@gmail.com changed reviewers: + franzih@chromium.org
Great, thanks so much! I have a few minor wording suggestions. And I think |this| notation doesn't work with Doxygen (at least not with my default config). Otherwise looks good. Oh, and commit message. We use [], capital letter, period at the end. (Basically the opposite of Node comments ;P ) You can edit that in the code review tool, no rebase needed. Something like this: [api] Add documentation to (Maybe)Locals methods. Add documentation to several methods and classes, including Local, MaybeLocal, and ... Drive-by-fix: turn a few regular comments into documentation comments. Thanks! https://codereview.chromium.org/2783843002/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode178 include/v8.h:178: * HandleScope must exist on the stack when they are created, and that they are I would change the wording a little bit: They are managed by HandleScopes. That means that a HandleScope must exist on the stack when the local handles are created and that ... I think no comma between created and and. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode180 include/v8.h:180: * For passing a local handle to an outer HandleScope, a EscapableHandleScope an EscapableH.... https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode181 include/v8.h:181: * and its Escape() method need to be used. Must instead of need https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode263 include/v8.h:263: * Casts a handle to a subclass, e.g. Local<Value> to Local<Object>. We are probably not consistent in this doc, but I'd prefer Cast a ... (without the s). https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode264 include/v8.h:264: * This is only valid if the handle actually refers to a value of the valid? The method throws otherwise, right? https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode277 include/v8.h:277: * Calling this is equivalent to Local<S>::Cast(). How about Alias for ...? https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode932 include/v8.h:932: * are allowed, which can be useful for debugging handle leaks. allowed. It can be useful... https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode2346 include/v8.h:2346: * The |kInternalized| flag acts as a hint that the string should be created There's Doxygen syntax for enums. Can we use that?
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Improve documentation comments in v8.h Add a few explanations to documentation comments in v8.h, and turn a few regular comments into documentation comments. BUG= ========== to ========== [api] Improve documentation for API handle types Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. Drive-by-fix: turn a few regular comments into documentation comments. ==========
On 2017/03/29 08:40:46, Franzi wrote: > Great, thanks so much! I have a few minor wording suggestions. And I think > |this| notation doesn't work with Doxygen (at least not with my default config). > Otherwise looks good. Yeah, but that’s what the header uses elsewhere, too. IIRC Doxygen needs <code> tags? It’s probably best to change that separately across the header then. > > Oh, and commit message. We use [], capital letter, period at the end. (Basically > the opposite of Node comments ;P ) > You can edit that in the code review tool, no rebase needed. Something like > this: > > [api] Add documentation to (Maybe)Locals methods. > > Add documentation to several methods and classes, including > Local, MaybeLocal, and ... > > Drive-by-fix: turn a few regular comments into documentation comments. > Done!
https://codereview.chromium.org/2783843002/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode178 include/v8.h:178: * HandleScope must exist on the stack when they are created, and that they are On 2017/03/29 08:40:45, Franzi wrote: > I would change the wording a little bit: > They are managed by HandleScopes. That means that a > HandleScope must exist on the stack when the local handles are created and that > ... > > I think no comma between created and and. Done. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode180 include/v8.h:180: * For passing a local handle to an outer HandleScope, a EscapableHandleScope On 2017/03/29 08:40:45, Franzi wrote: > an EscapableH.... Done. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode181 include/v8.h:181: * and its Escape() method need to be used. On 2017/03/29 08:40:45, Franzi wrote: > Must instead of need Done. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode263 include/v8.h:263: * Casts a handle to a subclass, e.g. Local<Value> to Local<Object>. On 2017/03/29 08:40:45, Franzi wrote: > We are probably not consistent in this doc, but I'd prefer > Cast a ... (without the s). Done. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode264 include/v8.h:264: * This is only valid if the handle actually refers to a value of the On 2017/03/29 08:40:45, Franzi wrote: > valid? The method throws otherwise, right? With V8_ENABLE_CHECKS it crashes, otherwise it fails silently. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode277 include/v8.h:277: * Calling this is equivalent to Local<S>::Cast(). On 2017/03/29 08:40:45, Franzi wrote: > How about > Alias for ...? It seems a bit odd to me to call a bound method and a static constructor aliases for each other... if you feel strongly about it, sure. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode932 include/v8.h:932: * are allowed, which can be useful for debugging handle leaks. On 2017/03/29 08:40:45, Franzi wrote: > allowed. It can be useful... Done. https://codereview.chromium.org/2783843002/diff/1/include/v8.h#newcode2346 include/v8.h:2346: * The |kInternalized| flag acts as a hint that the string should be created On 2017/03/29 08:40:45, Franzi wrote: > There's Doxygen syntax for enums. Can we use that? Done, assuming you meant adding doc comments to the enum values themselves?
Description was changed from ========== [api] Improve documentation for API handle types Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. Drive-by-fix: turn a few regular comments into documentation comments. ========== to ========== [api] Improve documentation for API handle types. Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. BUG= Drive-by-fix: turn a few regular comments into documentation comments. ==========
Description was changed from ========== [api] Improve documentation for API handle types. Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. BUG= Drive-by-fix: turn a few regular comments into documentation comments. ========== to ========== [api] Improve documentation for API handle types. Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. Drive-by-fix: turn a few regular comments into documentation comments. BUG= ==========
The CQ bit was checked by franzih@chromium.org
lgtm Usually Doxygen just links to anything it recognizes, sometimes that means you need to include namespaces, but no <code> tags or anything. But with our without |, the comments are still easy to understand. LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490811510294940,
"parent_rev": "891bbe2c858d9047b225ebbb4f89c56aff90574d", "commit_rev":
"a63744d50d0e8ee64446acc042b07708e69ec2ab"}
Message was sent while issue was closed.
Description was changed from ========== [api] Improve documentation for API handle types. Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. Drive-by-fix: turn a few regular comments into documentation comments. BUG= ========== to ========== [api] Improve documentation for API handle types. Add a few explanations to the documentation several methods and classes, in particular Local, MaybeLocal, the HandleScopes. Drive-by-fix: turn a few regular comments into documentation comments. BUG= Review-Url: https://codereview.chromium.org/2783843002 Cr-Commit-Position: refs/heads/master@{#44243} Committed: https://chromium.googlesource.com/v8/v8/+/a63744d50d0e8ee64446acc042b07708e69... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/a63744d50d0e8ee64446acc042b07708e69... |
