PPAPI: make generator output a pointer to an array correctly.

PPAPI: make generator output a pointer to an array correctly.

Originally an IDL argument like "[out] int[] foo" will generate
int* foo[]. It means an array of pointers to integers and what we
want is a pointer to an array of integers. Change the output to
int** foo to be not confusing.

BUG=387547
TEST=Run generator and build chrome.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291349

[wuchengli, 2014-08-19 07:40:36]

PTAL.

[Justin Chuang, 2014-08-19 17:31:13]

On second thought, I wonder if we can make generator to support:

    [out] PP_Size* jpeg_sizes);
    
The fix seems more general, and the syntax provides more flexibility. The above
line should be converted to:

    PP_Size** jpeg_sizes

There are many '[]' in IDLs. Look like most of these '[]' can still be kept in C
prototypes.
'[out] bar_t[] foo' is more like a special case.

https://codereview.chromium.org/490433002/diff/1/ppapi/generators/idl_c_proto.py
File ppapi/generators/idl_c_proto.py (right):

https://codereview.chromium.org/490433002/diff/1/ppapi/generators/idl_c_proto...
ppapi/generators/idl_c_proto.py:388: # type* foo[] means an array of pointers to
type, which is confusing.
nit: optionally elaborate more like this:

Function parameter with 'bar_t* foo[]' is ambiguous. It's legit for different
contexts:
   1. pass an array of pointers to bar_t objects.
   2. the caller prepares 'bar_t* ptr', and pass '&ptr'.
We only need the second context.

[Justin Chuang, 2014-08-19 17:33:49]

On 2014/08/19 17:31:13, Justin Chuang wrote:
> On second thought, I wonder if we can make generator to support:
> 
>     [out] PP_Size* jpeg_sizes);
>     

Need Dave's comments.

[dmichael (off chromium), 2014-08-19 21:43:21]

lgtm

https://codereview.chromium.org/490433002/diff/1/ppapi/generators/idl_c_proto.py
File ppapi/generators/idl_c_proto.py (right):

https://codereview.chromium.org/490433002/diff/1/ppapi/generators/idl_c_proto...
ppapi/generators/idl_c_proto.py:388: # type* foo[] means an array of pointers to
type, which is confusing.
On 2014/08/19 17:31:13, Justin Chuang wrote:
> nit: optionally elaborate more like this:
> 
> Function parameter with 'bar_t* foo[]' is ambiguous. It's legit for different
> contexts:
>    1. pass an array of pointers to bar_t objects.
>    2. the caller prepares 'bar_t* ptr', and pass '&ptr'.
> We only need the second context. 
I don't mean to come off as harsh, but I'm not sure what point you're trying to
make here. The IDL under discussion reads to me as "an array out-param", not an
"array of pointers":
[out, size_is(array_size)] PP_Size[] preview_sizes
...this currently generates to:
PP_Size* preview_sizes[]
...which is not what is the expressed intent from the IDL. I don't think it
matters whether that line of C has ambiguous intent or not; it's the wrong code
to generate based on the IDL.

If you're arguing that the _IDL_ is ambiguous, that's different. I think it
reads most clearly as "array out-param", so I think the fix is right,
personally.

[Justin Chuang, 2014-08-20 01:19:56]

On 2014/08/19 21:43:21, dmichael wrote:
> I don't mean to come off as harsh, but I'm not sure what point you're trying
to
> make here. The IDL under discussion reads to me as "an array out-param", not
an
> "array of pointers":
> [out, size_is(array_size)] PP_Size[] preview_sizes
> ...this currently generates to:
> PP_Size* preview_sizes[]
> ...which is not what is the expressed intent from the IDL. I don't think it
> matters whether that line of C has ambiguous intent or not; it's the wrong
code
> to generate based on the IDL.
> 
> If you're arguing that the _IDL_ is ambiguous, that's different. I think it
> reads most clearly as "array out-param", so I think the fix is right,
> personally.

I see. You are right. No point to elaborate wrongly-generated C code.

[wuchengli, 2014-08-20 05:49:56]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2014-08-20 05:50:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wuchengli@chromium.org/490433002/1

[commit-bot: I haz the power, 2014-08-20 06:49:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-20 06:52:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 06:52:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[wuchengli, 2014-08-22 03:05:14]

Commit failed because this CL changed C interface but not IDL. To work around
it, I'll change some comments of IDL. Let me know if you have other suggestions.

** Presubmit Warnings **
Missing PPAPI IDL for private interface, please generate IDL:

***************
  ppapi/c/private/ppb_camera_capabilities_private.h
  ppapi/c/private/ppp_flash_browser_operations.h
***************

[wuchengli, 2014-08-22 03:20:09]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2014-08-22 03:21:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wuchengli@chromium.org/490433002/40001

[commit-bot: I haz the power, 2014-08-22 04:49:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 05:42:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 05:42:44]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[wuchengli, 2014-08-22 05:51:07]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2014-08-22 05:51:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wuchengli@chromium.org/490433002/40001

[commit-bot: I haz the power, 2014-08-22 07:23:21]

Committed patchset #3 (40001) as 291349