Closed Bug 972385 Opened 10 years ago Closed 10 years ago

Make JS::AutoValueVector subscript operator return handles

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jonco, Assigned: mz_mhs-ctb)

References

Details

Attachments

(1 file, 10 obsolete files)

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
Attached patch WIP 1 (obsolete) — Splinter Review
So far, so good. Trying to figure out how to make changes in CodeGen.py to reflect this.
(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)
(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)
Attached patch WIP 2 (obsolete) — Splinter Review
Got CodeGen working, but I'm being plagued by unexplainable build failures.
Attached patch build-fixes (obsolete) — Splinter Review
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.
(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
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 [].
(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!
Attachment #8391971 - Attachment is obsolete: true
Attachment #8401021 - Attachment is obsolete: true
Attachment #8401374 - Attachment is obsolete: true
Attachment #8408772 - Flags: review?(luke)
Attachment #8408773 - Flags: review?(luke)
I think jonco or someone from the GC team would be more qualified to review this type of patch.
Attachment #8408772 - Flags: review?(luke) → review?(terrence)
Attachment #8408773 - Flags: review?(luke) → review?(terrence)
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)
Attachment #8408773 - Flags: review?(terrence) → review?(jcoppeard)
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)
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+
(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!
Attachment #8408772 - Attachment is obsolete: true
Attachment #8411434 - Flags: review?(jcoppeard)
Comment on attachment 8411434 [details] [diff] [review]
Part 1: Convert operator[] to Handle.

Oops. I guess I missed some.
Attachment #8411434 - Flags: review?(jcoppeard)
Attachment #8408773 - Attachment is obsolete: true
Attachment #8411437 - Flags: review?(jcoppeard)
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)
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+
Attachment #8411437 - Flags: review?(jcoppeard) → review+
Attachment #8411434 - Flags: review?(bugs)
Attachment #8411437 - Flags: review?(bugs)
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-
Attachment #8411437 - Flags: review?(bugs) → review+
Attached patch Patch (v3) (obsolete) — Splinter Review
Attachment #8411434 - Attachment is obsolete: true
Attachment #8411437 - Attachment is obsolete: true
Attachment #8412191 - Flags: review?(jcoppeard)
Attachment #8412191 - Flags: review?(bugs)
Attached patch subscript-operator.patch (obsolete) — Splinter Review
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)
So how is attachment 8412193 [details] [diff] [review] different from the patch I gave r+?
Attachment #8412193 - Flags: review?(bugs) → review+
I merged the two together (to avoid problems when bisecting) and you gave r- on the other one.
Comment on attachment 8412193 [details] [diff] [review]
subscript-operator.patch

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

Great!
Attachment #8412193 - Flags: review?(jcoppeard) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #8412193 - Attachment is obsolete: true
Keywords: checkin-needed
This is bitrotted. Please rebase.
Keywords: checkin-needed
Attached patch UnbitrottedSplinter Review
Attachment #8412915 - Attachment is obsolete: true
Keywords: checkin-needed
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.