Closed Bug 1144366 Opened 9 years ago Closed 9 years ago

Switch SpiderMonkey style from T *t to T* t

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed
firefox-esr31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(5 files, 8 obsolete files)

3.03 MB, patch
Details | Diff | Splinter Review
4.01 KB, text/x-python-script
Details
10.25 KB, text/x-python-script
jandem
: review+
Details
23.97 KB, patch
Details | Diff | Splinter Review
82.35 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
On IRC people agree this is something we want to do. It'll make life easier for people working on both Gecko and SM. We'll probably want to write a small script or use an existing tool to do this. We also want to change XPConnect at the same time, because it follows SM style.

Whatever we do should also work on patch files and aurora/beta branches.

bholley suggested using his scripts in bug 688012.
Attached file restyle.py - v1 (obsolete) —
This Python script is pretty effective. I scanned the output manually and it converts all interesting cases. I skimmed the output of a lot of files and didn't see anything obviously wrong. Of course it's hard to check a 15 MB patch completely.

It can be run as either:

- restyle.py --tree

This should be run from the root of a hg clone. Will restyle all JS/XPConnect/CTypes directories.

- restyle.py --files file1 file2

Will restyle file1, file2 etc. This also works on patch files, I checked it with some patches and they applied cleanly after running the script.
To be honest I prefer the "T *" notation for variable declaration over the "T* ".  The simple reason is that C and C++ are associating the "*" with the variable definition.  Even if this is a bad habit, and we should avoid confusing code like the following:

  char c, *pc;

as well as:

  char* pc, c;

Note that the second one is extremely ambiguous for human readers who are not used to C / C++.
So the full patch is 15 MB and I can't upload it uncompressed. Here's a smaller patch for just those directories, in case people are curious: 

js/src/asmjs/
js/src/vm
js/public/
Attached file restyle.py - v2 (obsolete) —
The restyle.py version I used to generate the attachment.
Attachment #8579274 - Attachment is obsolete: true
I can attach the full patch as .zip, but I'm not sure if that's useful because it will only apply to a few revisions. It's also huge, 15 MB.

I can post a patch per directory and request review on those parts. That's a bit more work, the individual patches will still be big and it won't be the exact same code I'll land due to other patches landing in the meantime.

Or somebody could run restyle.py locally and review/skim the changes it makes.
Flags: needinfo?(jorendorff)
From js/src/vm/NativeObject.h:

Changes in comments are not always correct:
---
/*
 * On success, and if id was found, return true with* objp non-null and with a
 * property of* objp stored in* propp. If successful but id was not found,
 * return true with both* objp and* propp null.
 */
---

Should this also be changed to `ObjectElements*` ?
---
static ObjectElements * fromElements(HeapSlot* elems) {
---

"ambiguous for human readers" per comment #2 ?
---
HeapSlot* fixedStart, *fixedEnd, *slotsStart, *slotsEnd;
---
(In reply to André Bargull from comment #6)
> Changes in comments are not always correct:

Hm yes there are a few of those cases, not many AFAICS. Many comments have examples or signatures we should update, so I don't want to skip comments completely. Maybe I can come up with some heuristics or at least detect the common cases: "in *p" etc.

> Should this also be changed to `ObjectElements*` ?
> ---
> static ObjectElements * fromElements(HeapSlot* elems) {
> ---

Yes this occurs a few times. I didn't change this because I'd need some way to distinguish it from a multiplication or bitwise and. Also: the code didn't match the old style so I'm not too worried about it not matching the new style either.

> "ambiguous for human readers" per comment #2 ?
> ---
> HeapSlot* fixedStart, *fixedEnd, *slotsStart, *slotsEnd;
> ---

I'm considering writing a script to detect those cases and either fix them automatically or just print a list so I can fix them up manually. I don't think it has to block this bug but it'd be nice as followup and shouldn't be hard to detect.
Attached file restyle.py - v3 (obsolete) —
I looked at all replacements in comments and added a whitelist of English words that are typically used, to avoid false positives like:

  Sets *foo to *bar. -> Set* foo to* bar.

I checked the interdiff and this fixes at least 120 false positives in comments (including the one André mentioned) and doesn't introduce any new problems.
Attachment #8579301 - Attachment is obsolete: true
Instead of changing JS to this nonstandard style only used by Gecko, could we change Gecko?
A quick search of the 10 most-starred C++ GitHub repositories shows 6/10 using "T* t" predominantly, with 1/10 using "T *t" and 2/10 switching between the two styles interchangeably.

Looks like I'm just too used to C.
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> To be honest I prefer the "T *" notation for variable declaration over the
> "T* ".

I do too, but i still think this is worth doing to match the rest of the project.

(In reply to Jan de Mooij [:jandem] from comment #5)
> Or somebody could run restyle.py locally and review/skim the changes it
> makes.

That seems best. I'll do it today.
Flags: needinfo?(jorendorff)
Attachment #8579517 - Flags: review?(jorendorff)
Attached file restyle.py - v4 (obsolete) —
This patch converts "Foo<Bar<baz> > *", this came up on IRC and was easy to fix. An interdiff shows only one hit in jspubtd.h:

    inline mozilla::LinkedList<PersistentRooted<Referent> > &getPersistentRootedList();

So not that urgent but we might as well handle it.
Attachment #8579517 - Attachment is obsolete: true
Attachment #8579517 - Flags: review?(jorendorff)
Attachment #8583356 - Flags: review?(jorendorff)
Well, here's everything I found. Of course there could be more. I'm separately writing a quite script to try to ensure that the tokens seen by C++ are unchanged.


In js/public/Class.h:
>- typedef JSObject *(*ClassObjectCreationOp)(JSContext* cx, JSProtoKey key);
>+ typedef JSObject*(*ClassObjectCreationOp)(JSContext* cx, JSProtoKey key);

Let's keep a space after the return type in a function-pointer type, so it looks like this:
   typedef JSObject* (*ClassObjectCreationOp)(JSContext* cx, JSProtoKey key);
That's consistent with what we do when the return type is not a pointer type, and it avoids messing up indentation in cases like this:

In js/public/StructuredClone.h:
>-typedef JSObject *(*ReadStructuredCloneOp)(JSContext* cx, JSStructuredCloneReader* r,
>+typedef JSObject*(*ReadStructuredCloneOp)(JSContext* cx, JSStructuredCloneReader* r,
>                                            uint32_t tag, uint32_t data, void* closure);

Careful not to regress |operator T *()| -> |operator T*()| while you're fixing this; I didn't see a test for it.

Function pointer types are not quite limited to typedefs; see at least DecompileFunctionSomehow in shell/js.cpp. Not sure if there are others.


In vm/RegExpObject.h:
>-    RegExpShared *operator->() { return re(); }
>+    RegExpShared* operator->() { return re(); }
>     RegExpShared &operator*() { return *re(); }

The `RegExpShared &operator*()` line should also be changed. There are a bunch of these.


In jsexn.h:
>- *     The original error described by *reportp typically won't be reported
>+ *     The original error described by* reportp typically won't be reported

Let's not change this one.


In perf/pm_linux.cpp:
>-        // For instance, I have *no idea* whether we should be setting
>+        // For instance, I have* no idea* whether we should be setting

And here.


In jsdtoa.cpp:
>-        else *p++ = '0';
>+        else* p++ = '0';

And here.


In jit/shared/IonAssemblerBuffer.h:
>         AssemblerBuffer_ *m_buffer;

This should be changed.


>         explicit AssemblerBufferInstIterator(BufferOffset off, AssemblerBuffer_ *buff)

And here (in the same file).


In jit/shared/BaseAssembler-x86-shared.h:
>-        spew("call*      %s", GPRegName(dst));
>+        spew("call*%s", GPRegName(dst));

This shouldn't change (6 places in that file).


In frontend/TokenStream.cpp:
>-    // "/\* //# sourceURL=<url> *\/"
>+    // "/\* //# sourceURL=<url>*\/"

Or this.


In frontend/Parser.cpp:
>+            // There is never a case in which |static* (| can make a meaningful method definition.
>-            // There is never a case in which |static*(| can make a meaningful method definition.

Or this.
(In reply to Jason Orendorff [:jorendorff] from comment #15)
> >+            // There is never a case in which |static* (| can make a meaningful method definition.
> >-            // There is never a case in which |static*(| can make a meaningful method definition.

Oops, got the + and - signs backwards. I manually reversed some hunks for that review, there may be more little mistakes like that. Should be clear enough though.
Attached file restyle.py - v5 (obsolete) —
Thanks for checking this so thoroughly! I added more tests, fixed all issues and some other minor things I noticed.

The script is also on github, in case people want to see the changes:

https://github.com/jandem/restyle/commits/master
Attachment #8583356 - Attachment is obsolete: true
Attachment #8583356 - Flags: review?(jorendorff)
Attachment #8584077 - Flags: review?(jorendorff)
Attached patch Interdiff (obsolete) — Splinter Review
The interdiff of the output from v4 and v5.
OK! No changed tokens!

But, the script should probably avoid making any changes in js/src/ctypes/libffi.

Also, I found a few changes in comments that should not be made:

In js/public/RootingAPI.h:
>-  *     (via &)
>+  *     (via&)

In js/src/asmjs/AsmJSValidate.cpp:
>-// *** asm.js FFI calls ***
>+// *** asm.js FFI calls***

In js/src/builtin/SymbolObject.cpp:
>-    // This uses &JSObject::class_ because: "The Symbol prototype object is an
>+    // This uses& JSObject::class_ because: "The Symbol prototype object is an

In js/src/frontend/BytecodeEmitter.cpp:
>-/* Other *NAME ops aren't (yet) supported in self-hosted code. */
>+/* Other* NAME ops aren't (yet) supported in self-hosted code. */

In js/src/frontend/BytecodeEmitter.h:
>-    // The caller should initialize *answer to false and invoke this function on
>+    // The caller should initialize* answer to false and invoke this function on

In js/src/frontend/Parser.cpp:
>-            // Handle the form |export *| by adding a special export batch
>+            // Handle the form |export*| by adding a special export batch

In js/src/jit/IonCaches.h (2 places):
>-//     IC    **|************                          |
>+//     IC**|************                          |

In js/src/jit/MoveResolver.cpp (2 places):
>-// Given move (A -> B), this function attempts to find any move (B -> *) in the
>+// Given move (A -> B), this function attempts to find any move (B ->*) in the

In js/src/jit/arm/CodeGenerator-arm.cpp:
>-// current instruction *PLUS 8*. This means that ldr foo, [pc, +0] reads
>+// current instruction* PLUS 8*. This means that ldr foo, [pc, +0] reads

In js/src/jit/shared/Assembler-shared.h:
>-// call *register
>+// call* register

In js/src/jit/shared/AssemblerBuffer-x86-shared.h
and again in BaseAssembler-x86-shared.h:
>-  * ***** BEGIN LICENSE BLOCK *****
>+  * ***** BEGIN LICENSE BLOCK*****

In js/src/jit/shared/BaseAssembler-x86-shared.h (7 places):
>- "call       *%s"
>+ "call*%s"

In js/src/jit/shared/Lowering-x86-shared.cpp:
>-//    movl          *mem, eax
>+//    movl*         mem, eax

In js/src/jit/x64/Lowering-x64.cpp:
>-//    movl          *mem, rax
>+//    movl*         mem, rax

In js/src/jit/x86/Lowering-x86.cpp:
>-//    movl          *mem, eax
>+//    movl*         mem, eax

In js/src/jsapi.h:
>-  * symbol; either way it is immune to GC so there is no need to visit *idp
>+  * symbol; either way it is immune to GC so there is no need to visit* idp

Also in js/src/jsapi.h (and this comment does not make any sense in the original, so... whatever):
>-      * Get the next value from the iterator.  If false *done is true
>+      * Get the next value from the iterator.  If false* done is true

In js/src/jsarray.cpp:
>- /* Create a new Array object and root it using *vp. */
>+ /* Create a new Array object and root it using* vp. */

In js/src/jsexn.h:
>-  *     The original error described by *reportp typically won't be reported
>+  *     The original error described by* reportp typically won't be reported

In js/src/jsfriendapi.h:
>-  * thus it will really return the outermost enclosing function *since the
>+  * thus it will really return the outermost enclosing function* since the
>   * innermost eval*.

In js/src/jsobj.h:
>- /*** Standard internal methods ********************************************************************
>+ /*** Standard internal methods********************************************************************

In js/src/jsscript.cpp:
>-  * Takes ownership of its *ssd parameter and either adds it into the runtime's
>+  * Takes ownership of its* ssd parameter and either adds it into the runtime's

In js/src/jsscript.h (this one is weird enough it should probably get a manual fixup or rewrite:
>- // Free variable names are possible tagged JSAtom *s.
>+ // Free variable names are possible tagged JSAtom* s.

In js/src/vm/Debugger.cpp:
>- // However, debug-mode OSR uses *this for both invalidating Ion frames,
>+ // However, debug-mode OSR uses* this for both invalidating Ion frames,

In js/src/vm/NativeObject.cpp:
>- // used in the spec: O -> pobj, P -> id, V -> *vp, ownDesc -> shape.
>+ // used in the spec: O -> pobj, P -> id, V ->* vp, ownDesc -> shape.

In js/src/vm/Shape.cpp:
>- /* FIXME bug 593129 -- slot allocation and JSObject *this must move out of here! */
>+ /* FIXME bug 593129 -- slot allocation and JSObject* this must move out of here! */
Comment on attachment 8584077 [details]
restyle.py - v5

This is good stuff.

Please do this during a slow time and make sure to announce it on dev.tech.js-engine.internals and dev.platform well in advance.
Attachment #8584077 - Flags: review?(jorendorff) → review+
Attached file check-restyle.py
For the curious, the script that checks token-for-token to see if anything changed.

Comments, weirdly, are treated as tokens by this script. The only output, when I run this after running jandem's script v4, is changed comments.

This is not based on the C++ standard and I wrote it in like 13 minutes.
(In reply to Jason Orendorff [:jorendorff] from comment #19)
> OK! No changed tokens!

I'll sleep better now, thanks for checking!

> But, the script should probably avoid making any changes in
> js/src/ctypes/libffi.

OK, I'll add a list of directories to ignore.

> Also, I found a few changes in comments that should not be made:

Comments are annoying to get right 100%. Tomorrow I'll update my blacklist and double-check the comment changes manually.

(In reply to Jason Orendorff [:jorendorff] from comment #20)
> Please do this during a slow time and make sure to announce it on
> dev.tech.js-engine.internals and dev.platform well in advance.

OK, will post today or tomorrow and then we can land it this weekend or early Monday morning, let me know if that's not "well in advance" enough :)
Attached file restyle.py - v6
This version blacklists libffi and fixes the remaining comment issues except those two:

(In reply to Jason Orendorff [:jorendorff] from comment #19)
> In js/src/jsscript.h (this one is weird enough it should probably get a
> manual fixup or rewrite:
> >- // Free variable names are possible tagged JSAtom *s.
> >+ // Free variable names are possible tagged JSAtom* s. 

> In js/src/vm/Shape.cpp:
> >- /* FIXME bug 593129 -- slot allocation and JSObject *this must move out of here! */
> >+ /* FIXME bug 593129 -- slot allocation and JSObject* this must move out of here! */

I don't think these changes are unreasonable or confusing (especially the last one) but I agree we should just rewrite them manually. Also note that it's NativeObject, not JSObject, nowadays, we should fix that too...
Attachment #8584077 - Attachment is obsolete: true
Attachment #8584425 - Flags: review+
Interdiff of the changes made by the script.
Attachment #8584079 - Attachment is obsolete: true
Attached patch Follow-up fixesSplinter Review
One pointer per declaration + rewrites the two comments mentioned before.
Attachment #8584472 - Flags: review?(jorendorff)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c030f97a04f

* I ran Jason's check-restyle script and verified it only reported comment changes.

* I verified qdiff -w is empty, this confirms the patch makes only whitespace changes and doesn't silently drop lines or something.
Comment on attachment 8584425 [details]
restyle.py - v6

Requesting approval to land this on Aurora and other branches, to make uplifts not a nightmare. We've done this before with similar changes, see bug 909499 for instance.

This will be a large patch, but it's automatically generated and we have a separate script to double check its correctness. The patch will only change code style (whitespace) and should not affect behavior at all.

If we don't backport this, uplifts will become a lot more error-prone and I think that's more likely to introduce (subtle) bugs than this change.

In a few days, beta will become release, so we don't need this there.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: will make uplifts a lot easier and less error-prone.
User impact if declined: -
Fix Landed on Version: 39
Risk to taking this patch (and alternatives if risky): This only changes code style and should not affect anything else.
String or UUID changes made by this patch: None.
Attachment #8584425 - Flags: approval-mozilla-esr31?
Attachment #8584425 - Flags: approval-mozilla-b2g37?
Attachment #8584425 - Flags: approval-mozilla-b2g34?
Attachment #8584425 - Flags: approval-mozilla-b2g32?
Attachment #8584425 - Flags: approval-mozilla-b2g30?
Attachment #8584425 - Flags: approval-mozilla-aurora?
Sorry, back to the rebase mines - had to back it out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5b892d8ef453 to get a clean backout of bustage from Friday afternoon.
(In reply to Phil Ringnalda (:philor) from comment #28)
> Sorry, back to the rebase mines - had to back it out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5b892d8ef453 to get a
> clean backout of bustage from Friday afternoon.

Relanded after discussing with philor on IRC:

https://hg.mozilla.org/integration/mozilla-inbound/rev/02f2f4c75007

Fingers crossed...
Comment on attachment 8584425 [details]
restyle.py - v6

We want to make life of developer easier wrt uplift (especially with 38 being an ESR release).
Attachment #8584425 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8584472 [details] [diff] [review]
Follow-up fixes

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

Yup. Thanks.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +496,1 @@
>          recovers_.writeInstruction(*it);

Please consider rewriting it this way:

    for (MNode* insn : *recover)
        recovers_.writeInstruction(insn);

I'm fine with pushing the patch you have, with or without this as a follow-up. This idiom where we're calling end() and then begin() on the next line reads kind of badly, that's all.

::: js/src/jsscript.cpp
@@ +320,5 @@
>      if (bindingArrayUsingTemporaryStorage())
>          return;
>  
> +    Binding* end = bindingArray() + count();
> +    for (Binding* b = bindingArray(); b != end; b++) {

class Bindings might as well get begin() and end() methods as well, right?

    for (Binding& b : *this) {
        ...
    }
Attachment #8584472 - Flags: review?(jorendorff) → review+
And the follow-up patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6ceba6f57e

Green on Linux64: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b85832a9e9a7

(In reply to Jason Orendorff [:jorendorff] from comment #33)
> Please consider rewriting it this way:

Good idea, range-based for loops make this much nicer. I also used it in some other places.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/fb6ceba6f57e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8584425 - Flags: approval-mozilla-esr31?
Attachment #8584425 - Flags: approval-mozilla-esr31+
Attachment #8584425 - Flags: approval-mozilla-b2g37?
Attachment #8584425 - Flags: approval-mozilla-b2g37+
Attachment #8584425 - Flags: approval-mozilla-b2g34?
Attachment #8584425 - Flags: approval-mozilla-b2g34+
Attachment #8584425 - Flags: approval-mozilla-b2g32?
Attachment #8584425 - Flags: approval-mozilla-b2g32+
Attachment #8584425 - Flags: approval-mozilla-b2g30?
Attachment #8584425 - Flags: approval-mozilla-b2g30+
What needs updating here is this page on wiki.mo:
https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style

Let me know, if we should move that into MDN, or feel free to move it yourself :)
(https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey is the main page.)

Note that the general Gecko coding style guide is on MDN, too: 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
NI myself to land this on ESR31 and b2g branches.
Flags: needinfo?(jdemooij)
Attachment #9040496 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.