Closed
Bug 1144366
Opened 10 years ago
Closed 10 years ago
Switch SpiderMonkey style from T *t to T* t
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
People
(Reporter: jandem, Assigned: jandem)
Details
Attachments
(6 files, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-python-script
|
Details | |
(deleted),
text/x-python-script
|
jandem
:
review+
Sylvestre
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-esr31+
bajaj
:
approval-mozilla-b2g30+
bajaj
:
approval-mozilla-b2g32+
bajaj
:
approval-mozilla-b2g34+
bajaj
:
approval-mozilla-b2g37+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/x-phabricator-request
|
Details |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
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++.
Assignee | ||
Comment 3•10 years ago
|
||
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/
Assignee | ||
Comment 4•10 years ago
|
||
The restyle.py version I used to generate the attachment.
Attachment #8579274 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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;
---
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
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
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8579294 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
Instead of changing JS to this nonstandard style only used by Gecko, could we change Gecko?
Comment 12•10 years ago
|
||
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.
Comment 13•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8579517 -
Flags: review?(jorendorff)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
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.
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
The interdiff of the output from v4 and v5.
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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+
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
(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 :)
Assignee | ||
Comment 23•10 years ago
|
||
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+
Assignee | ||
Comment 24•10 years ago
|
||
Interdiff of the changes made by the script.
Attachment #8584079 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
One pointer per declaration + rewrites the two comments mentioned before.
Attachment #8584472 -
Flags: review?(jorendorff)
Assignee | ||
Comment 26•10 years ago
|
||
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.
Keywords: dev-doc-needed,
leave-open
Assignee | ||
Comment 27•10 years ago
|
||
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?
Comment 28•10 years ago
|
||
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.
Assignee | ||
Comment 29•10 years ago
|
||
(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 30•10 years ago
|
||
Comment 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
status-firefox38:
--- → fixed
status-firefox39:
--- → fixed
Comment 33•10 years ago
|
||
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+
Assignee | ||
Comment 34•10 years ago
|
||
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
Comment 35•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•10 years ago
|
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+
Comment 36•10 years ago
|
||
dev-doc-info |
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
Assignee | ||
Comment 37•10 years ago
|
||
NI myself to land this on ESR31 and b2g branches.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 38•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr31/rev/e29f56074722
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/effd95861e9d
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a410efee499a
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b016448b6076
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9ab8a3ae0fc3
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox-esr31:
--- → fixed
Flags: needinfo?(jdemooij)
Comment 39•10 years ago
|
||
status-b2g-v2.1S:
--- → fixed
Comment 40•10 years ago
|
||
status-b2g-v2.0M:
--- → fixed
Comment 41•6 years ago
|
||
Updated•4 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•