Closed
Bug 1212663
Opened 9 years ago
Closed 9 years ago
Use doxygen style comments in jsapi
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: mwu, Assigned: mwu)
Details
Attachments
(1 file, 2 obsolete files)
141.97 KB,
patch
|
Details | Diff | Splinter Review |
The rust-bindgen fork used for SM bindings can find doxygen style comments and put them in the rust bindings. I converted a bunch of comments while testing that. The rustdoc output can be seen at file:///home/mikew/moz/rust-bindgen/target/debug/doc/jsapi/index.html
Assignee | ||
Updated•9 years ago
|
Attachment #8671092 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•9 years ago
|
||
The link should be https://people.mozilla.org/~mwu/doc/jsapi/
Comment 2•9 years ago
|
||
Might want to hold off until bug 1211607 lands (this one is probably the easier one to rebase).
Comment 3•9 years ago
|
||
As a note, this is somewhat in preparation for a second step: adding hints for rust-bindgen to the doxygen docs that make it understand some things it can't on its own: https://github.com/michaelwu/mozjs/commit/cbc562d045d4c96480c71778de6975117f76d4ea
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) > Might want to hold off until bug 1211607 lands (this one is probably the > easier one to rebase). Sure - I'll post a new one after that lands.
Comment 5•9 years ago
|
||
Comment on attachment 8671092 [details] [diff] [review] Convert jsapi comments to doxygen comments Review of attachment 8671092 [details] [diff] [review]: ----------------------------------------------------------------- Commenting in advance of the additional new commenting bits noted a comment or so prior. ::: js/public/Class.h @@ +250,5 @@ > // JSClass operation signatures. > > +/// Get a property named by id in obj. Note the jsid id type -- id may > +/// be a string (Unicode property identifier) or an int (element index). The > +/// *vp out parameter, on success, is the new property value after the action. Switch all the currently-// comments to /** */. Let's just be consistent on one thing, and on the thing best known (by Mozillians, at least, I think) about doxygen-supported syntax. ::: js/public/MemoryMetrics.h @@ +55,5 @@ > size_t other; > }; > > +/// These are the measurements used by Servo. It's important that this is a POD > +/// struct so that Servo can have a parallel |repr(C)| Rust equivalent. After this function, please add static_assert(mozilla::IsPod<ServoSizes>::value, "ServpSizes must be POD so that Servo can have a parallel " "|repr(C)| Rust equivalent"); and remove that bit of this comment. ::: js/public/Value.h @@ +237,4 @@ > > typedef enum JSWhyMagic > { > + JS_ELEMENTS_HOLE, /**< a hole in a native object's elements */ What's this < thing? Indicating the referent is to the left? Just move the comments above the initializers, add blank lines between the initializers, and be consistent with everything else. I doubt most Mozillians actually know this is doxygen syntax, let alone what it means.
Attachment #8671092 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5) > ::: js/public/MemoryMetrics.h > @@ +55,5 @@ > > size_t other; > > }; > > > > +/// These are the measurements used by Servo. It's important that this is a POD > > +/// struct so that Servo can have a parallel |repr(C)| Rust equivalent. > > After this function, please add > > static_assert(mozilla::IsPod<ServoSizes>::value, > "ServpSizes must be POD so that Servo can have a parallel " > "|repr(C)| Rust equivalent"); > > and remove that bit of this comment. > This is actually not true. We can convert most things that don't use sophisticated template features. Rules out a bunch of mfbt magic, but most of the interfaces we care about don't use the tricky stuff.
Assignee | ||
Comment 7•9 years ago
|
||
Review comments addressed, except for the POD thing in MemoryMetrics.h. I removed that part of the comment.
Attachment #8671092 -
Attachment is obsolete: true
Attachment #8674417 -
Flags: review?(jwalden+bmo)
Comment 8•9 years ago
|
||
Comment on attachment 8674417 [details] [diff] [review] Convert jsapi comments to doxygen comments, v2 Review of attachment 8674417 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Class.h @@ +852,4 @@ > ESClass_Boolean, ESClass_RegExp, ESClass_ArrayBuffer, ESClass_SharedArrayBuffer, > ESClass_Date, ESClass_Set, ESClass_Map, > > + /// None of the above. Make this /** */ too. ::: js/src/jsfriendapi.h @@ +2464,5 @@ > #undef JITINFO_OP_TYPE_BITS > > + /** Is op fallible? False in setters. */ > + uint32_t isInfallible : 1; > + /** Is op movable? To be movable the op must Add blank lines between these fields, and use /** * ... * ... */ style for any where the comments take up multple lines, e.g.: /** Is op fallible? False in setters. */ uint32_t isInfallible : 1; /** * Is op movable? To be movable the op must not AliasEverything, but even * that might not be enough (e.g. in cases when it can throw or is * explicitly not movable). */ uint32_t isMovable : 1;
Attachment #8674417 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Review comments addressed.
Attachment #8674417 -
Attachment is obsolete: true
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0e7e27c0872
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•