Closed
Bug 972385
Opened 10 years ago
Closed 10 years ago
Make JS::AutoValueVector subscript operator return handles
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jonco, Assigned: mz_mhs-ctb)
References
Details
Attachments
(1 file, 10 obsolete files)
44.54 KB,
patch
|
Details | Diff | Splinter Review |
Currently AutoVectorRooter and the classes derived from it have a subscript operator which returns a reference and a handleAt() method that returns a handle. We can make subscript return handles and use that everywhere.
Looking into this.
Assignee: nobody → mz_mhs-ctb
Status: NEW → ASSIGNED
So far, so good. Trying to figure out how to make changes in CodeGen.py to reflect this.
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Michael [:Earth4] from comment #2) Thanks for looking at this! Let me know if you have any questions.
Sorry to bother you, but do you know how I should change up Codegen.py to use .set(), etc. instead of assignment?
Flags: needinfo?(jcoppeard)
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Michael Shuen from comment #4) That will be tricky. My approach is usually to try building everything and see what fails, then try and work out where in the python the failing code is generated. If you post your latest patch I will try it and see if I can give you some pointers. Also, I'm on the #jsapi channel on IRC in European working hours so we can chat on there.
Flags: needinfo?(jcoppeard)
Got CodeGen working, but I'm being plagued by unexplainable build failures.
Reporter | ||
Comment 7•10 years ago
|
||
If you convert the last argument of MobileMessageManager::Send to be a MutableHandle, it builds fine for me. Here's a patch to do that.
Sadly, didn't work here. Keep getting syntax errors in the build system after linking libGLESv2.dll.
14:42.21 uriloader_exthandler.lib.desc
14:43.09 uriloader_prefetch.lib.desc
14:43.80 caps_src.lib.desc
14:46.30 Microsoft (R) File Expansion Utility Version 6.2.9200.16384
14:46.30 Copyright (c) Microsoft Corporation. All rights reserved.
14:46.30
14:46.30 Adding ../../dist/bin\D3DCompiler_43.dll to Extraction Queue
14:46.32
14:46.32 Expanding Files ....
14:46.32
14:46.32 Expanding Files Complete ...
14:46.53 libGLESv2.dll
>> 14:46.58 C:/Users/Michael/AppData/Local/Temp/make13576-1.sh: line 1: syntax erro
r near unexpected token `('
14:46.58 C:/Users/Michael/AppData/Local/Temp/make13576-1.sh: line 1: `c:/mozilla
-source/mozilla-central/obj-i686-pc-mingw32/_virtualenv/Scripts/python.exe c:/mo
zilla-source/mozilla-central/config/expandlibs_exec.py --depend .deps/libGLESv2.
dll.pp --target libGLESv2.dll --uselist -- link -NOLOGO -DLL -OUT:libGLESv2.dll
-PDB:libGLESv2.pdb -SUBSYSTEM:WINDOWS -MACHINE:X86 DiagnosticsBase.obj Directiv
eHandlerBase.obj DirectiveParser.obj ExpressionParser.obj Input.obj Lexer.obj Ma
You get the idea.
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Michael Shuen from comment #8) This doesn't look related to the changes - I think this may be a problem with your build setup. Can you try building without your patches applied to see if that works? I've pushed a try build of the patches here which will tell us for sure: https://tbpl.mozilla.org/?tree=Try&rev=3dd7326484a6
Assignee | ||
Comment 10•10 years ago
|
||
Yeah, it happens no matter what patches I build with. Do you know a way I can fix it? Also, I guess I'll start converting handleAt users to operator [].
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to Michael Shuen from comment #10) > Do you know a way I can fix it? It looks like it might be bug 990333 - do an update and try again. > Also, I guess I'll start converting handleAt users to operator []. That would be great too!
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8391971 -
Attachment is obsolete: true
Attachment #8401021 -
Attachment is obsolete: true
Attachment #8401374 -
Attachment is obsolete: true
Attachment #8408772 -
Flags: review?(luke)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8408773 -
Flags: review?(luke)
Comment 14•10 years ago
|
||
I think jonco or someone from the GC team would be more qualified to review this type of patch.
Updated•10 years ago
|
Attachment #8408772 -
Flags: review?(luke) → review?(terrence)
Updated•10 years ago
|
Attachment #8408773 -
Flags: review?(luke) → review?(terrence)
Comment 15•10 years ago
|
||
Comment on attachment 8408772 [details] [diff] [review] Part 1: Convert operator[] to Handle. Sounds like Jon has already been looking at these.
Attachment #8408772 -
Flags: review?(terrence) → review?(jcoppeard)
Updated•10 years ago
|
Attachment #8408773 -
Flags: review?(terrence) → review?(jcoppeard)
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8408772 [details] [diff] [review] Part 1: Convert operator[] to Handle. Review of attachment 8408772 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks good, the only thing is the use of handle.get() where handle.address() is what you really want. Can you fix the issues below and re-upload? ::: dom/bindings/BindingUtils.h @@ +2053,5 @@ > inline bool > AddStringToIDVector(JSContext* cx, JS::AutoIdVector& vector, const char* name) > { > return vector.growBy(1) && > + InternJSString(cx, (jsid&)(vector[vector.length() - 1].get()), name); Please change this to use *handle.address(), rather than using handle.get() and a c-style cast to get rid of the const. ::: js/src/ctypes/CTypes.cpp @@ +5135,5 @@ > > for (FieldInfoHash::Range r = fields->all(); !r.empty(); r.popFront()) { > const FieldInfoHash::Entry& entry = r.front(); > // Add the field descriptor to the array. > + if (!AddFieldToArray(cx, (jsval*)(&fieldsVec[entry.value().mIndex].get()), Please use handle.address() here rather than &handle.get(). @@ +5946,5 @@ > if (!vec.resize(len)) > return false; > > for (size_t i = 0; i < len; ++i) > + vec[i].set(JS::ObjectValue(*fninfo->mArgTypes[i])); You can do handle.setObject(*objPtr) so you don't need the JS::ObjectValue() here. @@ +6182,5 @@ > for (uint32_t i = 0; i < cif->nargs; ++i) { > // Convert each argument, and have any CData objects created depend on > // the existing buffers. > RootedObject argType(cx, fninfo->mArgTypes[i]); > + if (!ConvertToJS(cx, argType, NullPtr(), args[i], false, false, (jsval*)(&argv[i].get()))) Please use address() here. ::: js/src/jsscript.cpp @@ +234,5 @@ > for (uint32_t i = 0; i < nameCount; i++) { > RootedAtom atom(cx); > if (!XDRAtom(xdr, &atom)) > return false; > + atoms[i].set(StringValue(atom)); You can use handle.setString(atom) here. ::: js/src/jswrapper.cpp @@ +410,5 @@ > RootedValue v(cx, StringValue(ni->begin()[i])); > if (!ValueToId<CanGC>(cx, v, &id)) > return false; > keys.infallibleAppend(id); > + if (!origin->wrapId(cx, (jsid*)(&keys[i].get()))) Please use handle.address() here. ::: js/src/vm/Debugger.cpp @@ +5168,5 @@ > for (size_t i = 0; i < n; i++) { > if (!rewrappedIds.append(JSID_VOID) || !rewrappedDescs.append()) > return false; > id = ids[i]; > + if (!unwrappedDescs[i].wrapInto(cx, obj, id, (jsid*)(&rewrappedIds[i].get()), &rewrappedDescs[i])) Ditto here.
Attachment #8408772 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8408773 [details] [diff] [review] Part 2: Remove users of handleAt. Review of attachment 8408773 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8408773 -
Flags: review?(jcoppeard) → review+
Reporter | ||
Comment 18•10 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #17) I found there's another use of handleAt in ToJSValue() in dom/bindings/ToJSValue.h - please convert this one too. Cheers!
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8408772 -
Attachment is obsolete: true
Attachment #8411434 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8411434 [details] [diff] [review] Part 1: Convert operator[] to Handle. Oops. I guess I missed some.
Attachment #8411434 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8408773 -
Attachment is obsolete: true
Attachment #8411437 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8411434 [details] [diff] [review] Part 1: Convert operator[] to Handle. Well, it seems to be fine. I was a little worried about .get() usage in a few spots, but it seems to be OK.
Attachment #8411434 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8411434 [details] [diff] [review] Part 1: Convert operator[] to Handle. Review of attachment 8411434 [details] [diff] [review]: ----------------------------------------------------------------- This all looks good. I saw that there were some changes to CTypes.cpp in the second patch that should be part of this patch. That's fine but please combine the two patches into one before pushing, otherwise people trying to bisect will trip over the broken revision. Next we need to get the browser parts reviewed. ::: dom/bindings/Codegen.py @@ +12030,5 @@ > " else if (argc == %d) {\n" > " // This is our current trailing argument; reduce argc\n" > " --argc;\n" > "} else {\n" > + " argv[%d].set(JS::UndefinedValue());\n" You can use setUndefined() for this. ::: js/src/ctypes/CTypes.cpp @@ +4827,5 @@ > RootedObject fieldType(cx, nullptr); > Rooted<JSFlatString*> name(cx, ExtractStructField(cx, item, fieldType.address())); > if (!name) > return false; > + fieldRoots[i].set(JS::ObjectValue(*fieldType)); You can use setObject() here.
Attachment #8411434 -
Flags: review?(jcoppeard) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8411437 -
Flags: review?(jcoppeard) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8411434 -
Flags: review?(bugs)
Reporter | ||
Updated•10 years ago
|
Attachment #8411437 -
Flags: review?(bugs)
Comment 24•10 years ago
|
||
Comment on attachment 8411434 [details] [diff] [review] Part 1: Convert operator[] to Handle. This patch seems to have some random tab characters. Should use only spaces for indentation.
Attachment #8411434 -
Flags: review?(bugs) → review-
Updated•10 years ago
|
Attachment #8411437 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8411434 -
Attachment is obsolete: true
Attachment #8411437 -
Attachment is obsolete: true
Attachment #8412191 -
Flags: review?(jcoppeard)
Attachment #8412191 -
Flags: review?(bugs)
Assignee | ||
Comment 26•10 years ago
|
||
Pulling tip broke the patch apply. Fixed that.
Attachment #8412191 -
Attachment is obsolete: true
Attachment #8412191 -
Flags: review?(jcoppeard)
Attachment #8412191 -
Flags: review?(bugs)
Attachment #8412193 -
Flags: review?(jcoppeard)
Attachment #8412193 -
Flags: review?(bugs)
Comment 27•10 years ago
|
||
So how is attachment 8412193 [details] [diff] [review] different from the patch I gave r+?
Updated•10 years ago
|
Attachment #8412193 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 28•10 years ago
|
||
I merged the two together (to avoid problems when bisecting) and you gave r- on the other one.
Reporter | ||
Comment 29•10 years ago
|
||
Comment on attachment 8412193 [details] [diff] [review] subscript-operator.patch Review of attachment 8412193 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #8412193 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8412193 -
Attachment is obsolete: true
Keywords: checkin-needed
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8412915 -
Attachment is obsolete: true
Keywords: checkin-needed
Comment 33•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed9e9e160f2
Keywords: checkin-needed
Comment 34•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6ed9e9e160f2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•