Bug 158529 - Improve code generator for functions with variadic parameters
Summary: Improve code generator for functions with variadic parameters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nael Ouedraogo
URL:
Keywords:
Depends on: 158942
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-08 09:25 PDT by Nael Ouedraogo
Modified: 2016-06-20 10:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (14.81 KB, patch)
2016-06-08 09:39 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch for landing (15.62 KB, patch)
2016-06-13 06:07 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nael Ouedraogo 2016-06-08 09:25:41 PDT
Improve code generator for functions with variadic parameters.
Comment 1 Nael Ouedraogo 2016-06-08 09:39:33 PDT
Created attachment 280811 [details]
Patch
Comment 2 Nael Ouedraogo 2016-06-08 09:41:11 PDT
JS bindings code generated with variadic parameters can be improved as proposed in the uploaded patch.
A further possible improvement is to avoid using a Vector of pointers since each item is checked to be not null before being append to the Vector. I would like to use a Vector of "references" instead.
One solution would be to use a Vector of std::reference_wrapper<T>. Is there another solution ?

It seems that variadic parameters are not correctly supported in ObjC and GObject code generators. A test is added to skip functions with variadic.
Comment 3 Darin Adler 2016-06-09 09:35:02 PDT
Comment on attachment 280811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280811&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3666
> +                push(@$outputArray, "    $name.reserveInitialCapacity(state->argumentCount() - $argumentIndex);\n");

Do we have an ironclad guarantee that argumentCount() is >= $argumentIndex? It‘s critical that we know that’s guaranteed, because if not this will try to reserve a colossal capacity.
Comment 4 Nael Ouedraogo 2016-06-10 08:21:07 PDT
> Do we have an ironclad guarantee that argumentCount() is >= $argumentIndex?
> It‘s critical that we know that’s guaranteed, because if not this will try
> to reserve a colossal capacity.

Thanks for the review. 

Actually, we have no guarantee. It may happen when a variadic parameter is preceded by an optional parameter. This issue is also observed in toNativeArguments(). 

I will modify JS code generator so that it dies when an optional parameter is preceding a variadic. I will also add an assert before call to reserveInitialCapacity().

I am going to land a new patch with these modifications.
Comment 5 Nael Ouedraogo 2016-06-13 06:07:45 PDT
Created attachment 281170 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-06-13 06:38:50 PDT
Comment on attachment 281170 [details]
Patch for landing

Clearing flags on attachment: 281170

Committed r201988: <http://trac.webkit.org/changeset/201988>
Comment 7 WebKit Commit Bot 2016-06-13 06:38:55 PDT
All reviewed patches have been landed.  Closing bug.