Closed Bug 1471272 Opened Last year Closed Last year

Add [SMDOC] tag for in-source SpiderMonkey documentation

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I've run this idea by a few people already without strong objection but now have gotten around to doing it.

This adds [SMDOC] markers with a one line description to various comment docs in the codebase. We have a lot of good information hidden away in tree and this is a simple way to make it easier to find. Devs (new and old) can grep or searchfox for the tag to find the comment blocks we have.

This is an initial collection of them. People should tag more things they find valuable and add comment docs for systems that lack them. As we build up the set, we may evolve the naming convention to something more hierarchical (suggestions welcome!).
Attachment #8987864 - Flags: review?(jdemooij)
Comment on attachment 8987864 [details] [diff] [review]
0001-WIP-Add-SMDOC-tags-for-in-source-documentation.patch

Review of attachment 8987864 [details] [diff] [review]:
-----------------------------------------------------------------

Really good idea! A small effort of indexing, and a simple way to grep for documentation.

nit:
https://searchfox.org/mozilla-central/source/js/src/jit/Recover.h#20 (Recover Instruction)
https://searchfox.org/mozilla-central/source/js/src/jit/MacroAssembler.h#44 (MacroAssembler meta-doc)
https://searchfox.org/mozilla-central/source/js/src/jit/RegisterSets.h#469 (Register Sets)
https://searchfox.org/mozilla-central/source/js/src/jit/RegisterSets.h#507 (Allocatable Register Sets)
https://searchfox.org/mozilla-central/source/js/src/jit/RegisterSets.h#623 (Live Register Sets)
Attachment #8987864 - Flags: review+
Updated with feedback from Comment 1
Attachment #8987864 - Attachment is obsolete: true
Attachment #8987864 - Flags: review?(jdemooij)
Attachment #8987914 - Flags: review?(jdemooij)
Comment on attachment 8987914 [details] [diff] [review]
0001-WIP-Add-SMDOC-tags-for-in-source-documentation.patch

Review of attachment 8987914 [details] [diff] [review]:
-----------------------------------------------------------------

Fantastic, thanks for doing this!

Please add the following:

* gc/Zone.h (line 102)
* jit/CacheIR.h (line 25)
* vm/ArgumentsObject.h (line 100)
* vm/Runtime.h (line 110)
* vm/Stack.h (line 72)

(Also maybe jit/ValueNumbering.cpp (line 17) and AliasAnalysis.cpp (line 120) if you think they qualify.)

::: js/public/CallArgs.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /*
> + * [SMDOC] JS::CallArgs API

Please remove this part of the comment (these APIs are gone):

 * It's also possible to use the supported API of JS_CALLEE, JS_THIS, JS_ARGV,
 * JS_RVAL, and JS_SET_RVAL to the same ends.

Also the "neither" in the next sentence should be fixed then.

::: js/src/jit/Bailouts.h
@@ +21,1 @@
>  // A "bailout" is a condition in which we need to recover an interpreter frame

Nit: s/an interpreter frame/a baseline frame/ here and at least once below.

(Actually it can be multiple Baseline frames because of inlining..)
Attachment #8987914 - Flags: review?(jdemooij) → review+
Last ones: gc/AtomMarking.cpp has an overview comment and vm/SymbolType.h (line 125).
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67c0442a7a44
Add [SMDOC] tags for in-source documentation. r=jandem,nbp
https://hg.mozilla.org/mozilla-central/rev/67c0442a7a44
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.