default_abi.toSource() is useless

RESOLVED FIXED in mozilla15

Status

()

Core
js-ctypes
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: sawrubh)

Tracking

unspecified
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=c++])

Attachments

(2 attachments, 2 obsolete attachments)

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 "({})".
Actually, I have a workaround, so removing the "Blocks". But it would still be nicer if I did not have to use this workaround.
No longer blocks: 563742
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [mentor=Yoric][lang=c++]

Comment 2

5 years ago
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.
Of course, it should also work with other ABIs.
(Assignee)

Comment 4

5 years ago
Please assign it to me. I want to/am working on this.
Assignee: nobody → saurabhanandiit
(Assignee)

Comment 5

5 years ago
Created attachment 623143 [details] [diff] [review]
Patch v1
Attachment #623143 - Flags: review?(dteller)
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;
}
Attachment #623143 - Flags: review?(dteller)
(Assignee)

Comment 7

5 years ago
Created attachment 623167 [details] [diff] [review]
Patch v2
Attachment #623143 - Attachment is obsolete: true
Attachment #623167 - Flags: review?(jorendorff)
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.)
Attachment #623167 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 9

5 years ago
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.
Attachment #623167 - Attachment is obsolete: true
Attachment #623996 - Flags: review?(jorendorff)
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?
Attachment #623996 - Flags: feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 624008 [details] [diff] [review]
Patch v4
Attachment #623996 - Attachment is obsolete: true
Attachment #624008 - Flags: review?(jorendorff)
Attachment #623996 - Flags: review?(jorendorff)
(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 on attachment 623996 [details] [diff] [review]
Patch v3

Great. I'll land it tonight or tomorrow.
Attachment #623996 - Flags: review+
(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.
Attachment #624008 - Flags: review?(jorendorff)
Attachment #623996 - Attachment is obsolete: false
https://hg.mozilla.org/integration/mozilla-inbound/rev/b697bb3d3846
https://hg.mozilla.org/mozilla-central/rev/b697bb3d3846
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.