Last Comment Bug 732262 - default_abi.toSource() is useless
: default_abi.toSource() is useless
Status: RESOLVED FIXED
[mentor=Yoric][lang=c++]
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Saurabh Anand [:sawrubh]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-01 16:22 PST by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-05-16 19:52 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (5.24 KB, patch)
2012-05-11 08:04 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v2 (5.08 KB, patch)
2012-05-11 08:54 PDT, Saurabh Anand [:sawrubh]
jorendorff: review+
Details | Diff | Splinter Review
Patch v3 (5.12 KB, patch)
2012-05-15 04:35 PDT, Saurabh Anand [:sawrubh]
jorendorff: review+
dteller: feedback+
Details | Diff | Splinter Review
Patch v4 (5.08 KB, patch)
2012-05-15 05:53 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-03-01 16:22:35 PST
For serialization purposes, I would like the ability to perform |ctypes.default_abi.toSource()|. This should return "ctypes.default_abi". Unfortunately, at the moment, this returns "({})".
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-03-01 16:25:56 PST
Actually, I have a workaround, so removing the "Blocks". But it would still be nicer if I did not have to use this workaround.
Comment 2 Josh Matthews [:jdm] 2012-05-04 01:18:32 PDT
http://hg.mozilla.org/mozilla-central/file/5aa794f7d5cd/js/src/ctypes/CTypes.cpp#l804 is the code that sets up each ABI object. You will need to add code similar to http://hg.mozilla.org/mozilla-central/file/5aa794f7d5cd/js/src/ctypes/CTypes.cpp#l788 which defines a set of functions on a JS object; the functions this case will be a modified version of http://hg.mozilla.org/mozilla-central/file/5aa794f7d5cd/js/src/ctypes/CTypes.cpp#l482 that only includes a new toString method that you will need to write. In this method, you will use the value that is stored at http://hg.mozilla.org/mozilla-central/file/5aa794f7d5cd/js/src/ctypes/CTypes.cpp#l808 to determine which ABI string to return.
Comment 3 David Teller [:Yoric] (please use "needinfo") 2012-05-10 04:50:52 PDT
Of course, it should also work with other ABIs.
Comment 4 Saurabh Anand [:sawrubh] 2012-05-10 08:45:38 PDT
Please assign it to me. I want to/am working on this.
Comment 5 Saurabh Anand [:sawrubh] 2012-05-11 08:04:05 PDT
Created attachment 623143 [details] [diff] [review]
Patch v1
Comment 6 David Teller [:Yoric] (please use "needinfo") 2012-05-11 08:11:10 PDT
Comment on attachment 623143 [details] [diff] [review]
Patch v1

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

Looks good. Let me suggest a few minor changes.
Once you have applied them, ask a review from jorendorff.

::: js/src/ctypes/CTypes.cpp
@@ +779,5 @@
> +    return obj;
> +  } else {
> +    return NULL;
> +  }
> +      

Typically, we write this as follows:

if (!JS_DefineFunctions(cx, obj, sCABIFunctions)) {
  return NULL;
}
return obj;

@@ +3614,5 @@
> +  } else {
> +    JS_ReportError(cx, "not a valid ABICode");
> +    return JS_FALSE;
> +  }
> +  

You could group that |if| and that |switch|.

JSString* result;
switch(GetABICode(obj)) {
  // ...
  default:
     JS_ReportError(...)
     return JS_FALSE;
}
Comment 7 Saurabh Anand [:sawrubh] 2012-05-11 08:54:42 PDT
Created attachment 623167 [details] [diff] [review]
Patch v2
Comment 8 Jason Orendorff [:jorendorff] 2012-05-14 15:18:02 PDT
Comment on attachment 623167 [details] [diff] [review]
Patch v2

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

r=me with the two changes and a few trivial whitespace changes (just to conform to the surrounding style in this file).

I'll land this, but not tonight.

::: js/src/ctypes/CTypes.cpp
@@ +457,5 @@
>  #define CTYPESPROP_FLAGS \
>    (JSPROP_SHARED | JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT)
>  
> +#define CABIFN_FLAGS \
> +  (JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT)

Remove JSPROP_ENUMERATE please. Methods normally aren't enumerable.

@@ +482,5 @@
>  };
>  
> +static JSFunctionSpec sCABIFunctions[] = {
> +  JS_FN("toSource", ABI::ToSource, 0, CABIFN_FLAGS),
> +  JS_FS_END

Might as well add a "toString" entry here, too. (The JS language implicitly calls .toString() in many places.)
Comment 9 Saurabh Anand [:sawrubh] 2012-05-15 04:35:16 PDT
Created attachment 623996 [details] [diff] [review]
Patch v3

I have made the following changes :
a) Removed unnecessary white spaces
b) Removed JSPROP_ENUMERATE
c) Added/linked "toString" to ABI::ToSource.
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-05-15 05:46:59 PDT
Comment on attachment 623996 [details] [diff] [review]
Patch v3

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

One more nitpick :)

::: js/src/ctypes/CTypes.cpp
@@ +3608,5 @@
> +      JS_ReportError(cx, "not a valid ABICode");
> +      return JS_FALSE;
> +  }
> +  if (!result)
> +    return JS_FALSE;

This last check is redundant, isn't it?
Comment 11 Saurabh Anand [:sawrubh] 2012-05-15 05:53:11 PDT
Created attachment 624008 [details] [diff] [review]
Patch v4
Comment 12 Jason Orendorff [:jorendorff] 2012-05-15 13:35:22 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> ::: js/src/ctypes/CTypes.cpp
> @@ +3608,5 @@
> > +      JS_ReportError(cx, "not a valid ABICode");
> > +      return JS_FALSE;
> > +  }
> > +  if (!result)
> > +    return JS_FALSE;
> 
> This last check is redundant, isn't it?

I don't think it was redundant. JS_NewStringCopyZ can fail.
Comment 13 Jason Orendorff [:jorendorff] 2012-05-15 13:36:48 PDT
Comment on attachment 623996 [details] [diff] [review]
Patch v3

Great. I'll land it tonight or tomorrow.
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-05-16 06:05:54 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #12)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> > ::: js/src/ctypes/CTypes.cpp
> > @@ +3608,5 @@
> > > +      JS_ReportError(cx, "not a valid ABICode");
> > > +      return JS_FALSE;
> > > +  }
> > > +  if (!result)
> > > +    return JS_FALSE;
> > 
> > This last check is redundant, isn't it?
> 
> I don't think it was redundant. JS_NewStringCopyZ can fail.

You are right, my bad.
Comment 15 Jason Orendorff [:jorendorff] 2012-05-16 07:49:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b697bb3d3846
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-05-16 19:52:34 PDT
https://hg.mozilla.org/mozilla-central/rev/b697bb3d3846

Note You need to log in before you can comment on or make changes to this bug.