Last Comment Bug 723806 - JS CTypes prints function types incorrectly
: JS CTypes prints function types incorrectly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 19:13 PST by Jim Blandy :jimb
Modified: 2012-02-15 09:03 PST (History)
3 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make JavaScript CTypes print function types correctly. (5.44 KB, patch)
2012-02-02 19:13 PST, Jim Blandy :jimb
bobbyholley: review+
Details | Diff | Splinter Review
Make JavaScript CTypes print function types correctly. (r=bholley on original) (7.42 KB, patch)
2012-02-06 16:01 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Make JavaScript CTypes print function types correctly. (r=bholley on original) (8.45 KB, patch)
2012-02-09 23:50 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review

Description Jim Blandy :jimb 2012-02-02 19:13:34 PST
Created attachment 594054 [details] [diff] [review]
Make JavaScript CTypes print function types correctly.

CTypes prints function types incorrectly. For example:

js> ctypes.FunctionType(ctypes.default_abi, ctypes.int).toString()
"type int()()"
js> 

It should return "type int()".
Comment 1 Jim Blandy :jimb 2012-02-02 19:18:18 PST
I thought Chris had helped work on CTypes, and thus r?'d him; if that's not correct, apologies; who is best?
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-02-04 16:33:08 PST
Comment on attachment 594054 [details] [diff] [review]
Make JavaScript CTypes print function types correctly.

>-      // Wrap the entire expression so far with parens.
>-      PrependString(result, "(");
>-      AppendString(result, ")");
>+      // Function application binds more tightly than dereferencing, so
>+      // wrap pointer types in parens.
>+      if (prevGrouping == TYPE_pointer) {
>+        PrependString(result, "(");
>+        AppendString(result, ")");
>+      }

Good catch. :-)

Could be worth making a note here that pointers are the only modifiers that can directly wrap functions (since you can't have arrays of functions or functions returning functions), so the only thing we're guarding against here is function types as the outermost modifier. But it's not super crucial.


>diff --git a/js/src/jit-test/tests/basic/CTypes-toString.js b/js/src/jit-test/tests/basic/CTypes-toString.js

Awesome tests! These should go in toolkit/components/ctypes/tests though.

r=bholley with the tests moved.
Comment 3 Jim Blandy :jimb 2012-02-06 16:01:07 PST
Created attachment 594854 [details] [diff] [review]
Make JavaScript CTypes print function types correctly. (r=bholley on original)

Revised per bholley's review comments.
Comment 4 Jim Blandy :jimb 2012-02-06 16:05:38 PST
try run: https://tbpl.mozilla.org/?tree=Try&rev=4eee592f6378
Comment 5 Jim Blandy :jimb 2012-02-06 21:19:49 PST
Try looks good.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0077ffef801
Comment 6 Marco Bonardo [::mak] 2012-02-07 01:19:00 PST
Backed out due to test failures on all Windows boxes in test_ctypes.js and test_ctypes.xul.

Fwiw, Try didn't really look good ;)
Comment 7 Jim Blandy :jimb 2012-02-08 10:47:01 PST
(In reply to Marco Bonardo [:mak] from comment #6)
> Fwiw, Try didn't really look good ;)

I suck. Thanks, Marco.
Comment 8 Jim Blandy :jimb 2012-02-09 23:50:45 PST
Created attachment 595983 [details] [diff] [review]
Make JavaScript CTypes print function types correctly. (r=bholley on original)

Revised to fix Win32 tests (bad spacing for __stdcall and WINAPI qualifiers).
Comment 9 Jim Blandy :jimb 2012-02-09 23:51:03 PST
New try run: https://tbpl.mozilla.org/?tree=Try&rev=c327706ed342
Comment 10 Jim Blandy :jimb 2012-02-14 12:18:55 PST
(In reply to Jim Blandy :jimb from comment #9)
> New try run: https://tbpl.mozilla.org/?tree=Try&rev=c327706ed342

That showed up some more xpcshell test failures. This one is clean:
https://tbpl.mozilla.org/?tree=Try&rev=cc27c2c3fa1a
Comment 12 Marco Bonardo [::mak] 2012-02-15 09:03:46 PST
https://hg.mozilla.org/mozilla-central/rev/f80c406b902a

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