Closed Bug 1212663 Opened 9 years ago Closed 9 years ago

Use doxygen style comments in jsapi

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mwu, Assigned: mwu)

Details

Attachments

(1 file, 2 obsolete files)

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
Attachment #8671092 - Flags: review?(jwalden+bmo)
The link should be https://people.mozilla.org/~mwu/doc/jsapi/
Might want to hold off until bug 1211607 lands (this one is probably the easier one to rebase).
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
(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 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)
(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.
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 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+
Review comments addressed.
Attachment #8674417 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a0e7e27c0872
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.