Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9390009:
Implement C++ bindings generation in dartgenerator.py. (Closed)
|
Created:
8 years, 10 months ago by podivilov Modified:
8 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionImplement C++ bindings generation in dartgenerator.py.
R=antonm@google.com,sra@google.com
Committed: https://code.google.com/p/dart/source/detail?r=4256
Patch Set 1 #
Total comments: 36
Patch Set 2 : Comments addressed. #
Total comments: 11
Messages
Total messages: 6 (0 generated)
Pretty neat, first round of comments https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... File client/dom/scripts/dartdomgenerator.py (right): https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartdomgenerator.py:59: generator.ConvertToDartTypes(webkit_database) I am not acquainted with this stuff, but why clone after conversion? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... File client/dom/scripts/dartgenerator.py (right): https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:532: generators = [g for g in generators if g] filter(generators) might be more readable in this context even though comprehensions are cute. https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2236: has_dart_wrapper=True,conversion_template=''): nit: space after , https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2247: if self._native_type != '': nit: None might be more pythonic variant of missing value. https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2266: return '' return None? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2278: if self._conversion_template != '': nit: same for None https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2296: if self._native_type == 'String': _native_type or native_type() ? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2301: return '' return None? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2308: self._native_type = native_type why this assignment? maybe pass native_type instead? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2325: 'boolean': PrimitiveIDLTypeInfo('boolean', native_type='bool', conversion_template='static_cast<bool>(%s)'), why cast? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2334: 'unsigned short': PrimitiveIDLTypeInfo('unsigned short', native_type='int', conversion_template='static_cast<int>(%s)'), please, provide comments here and for short why casts are needed. https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2368: if idl_type in _original_idl_types: those 4 lines can be compressed to _original_idl_types.get(idl_type, idl_type.id) https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2375: if idl_type_name in _idl_type_registry: ditto https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2467: ' $RETURN_STATEMENT\n' maybe provide ; here and not at ln. 2447 https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2634: self._interface.id in ['FileReader', 'WebKitCSSMatrix']): maybe put this information into type registry? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2670: self._interface.id in ['DOMWindow', 'Element', 'HTMLElement', 'SVGElement']): ditto for type registry https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2731: idl_type = GetIDLTypeInfo(attr.type).idl_type() again, type registry? https://chromiumcodereview.appspot.com/9390009/diff/1/client/dom/scripts/dart... client/dom/scripts/dartgenerator.py:2947: ' v8::HandleScope handleScope;\n' separate template file?
Thanks for the comments! PTAL. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartdomgener... File client/dom/scripts/dartdomgenerator.py (right): http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartdomgener... client/dom/scripts/dartdomgenerator.py:59: generator.ConvertToDartTypes(webkit_database) On 2012/02/13 18:52:46, antonm wrote: > I am not acquainted with this stuff, but why clone after conversion? Cloned idl types don't appear in _original_idl_types map in dartgenerator.py. Added fixme to revert this stuff later. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... File client/dom/scripts/dartgenerator.py (right): http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:532: generators = [g for g in generators if g] On 2012/02/13 18:52:46, antonm wrote: > filter(generators) might be more readable in this context even though > comprehensions are cute. Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2236: has_dart_wrapper=True,conversion_template=''): On 2012/02/13 18:52:46, antonm wrote: > nit: space after , Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2247: if self._native_type != '': On 2012/02/13 18:52:46, antonm wrote: > nit: None might be more pythonic variant of missing value. Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2266: return '' On 2012/02/13 18:52:46, antonm wrote: > return None? Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2278: if self._conversion_template != '': On 2012/02/13 18:52:46, antonm wrote: > nit: same for None Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2296: if self._native_type == 'String': On 2012/02/13 18:52:46, antonm wrote: > _native_type or native_type() ? agreed, native_type() is better http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2301: return '' On 2012/02/13 18:52:46, antonm wrote: > return None? Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2308: self._native_type = native_type On 2012/02/13 18:52:46, antonm wrote: > why this assignment? maybe pass native_type instead? Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2325: 'boolean': PrimitiveIDLTypeInfo('boolean', native_type='bool', conversion_template='static_cast<bool>(%s)'), On 2012/02/13 18:52:46, antonm wrote: > why cast? There is GC3Dboolean which is not a bool, but unsigned char for OpenGL compatibility. Added a comment. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2334: 'unsigned short': PrimitiveIDLTypeInfo('unsigned short', native_type='int', conversion_template='static_cast<int>(%s)'), On 2012/02/13 18:52:46, antonm wrote: > please, provide comments here and for short why casts are needed. Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2368: if idl_type in _original_idl_types: On 2012/02/13 18:52:46, antonm wrote: > those 4 lines can be compressed to _original_idl_types.get(idl_type, > idl_type.id) That's nice! http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2375: if idl_type_name in _idl_type_registry: On 2012/02/13 18:52:46, antonm wrote: > ditto Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2467: ' $RETURN_STATEMENT\n' On 2012/02/13 18:52:46, antonm wrote: > maybe provide ; here and not at ln. 2447 Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2634: self._interface.id in ['FileReader', 'WebKitCSSMatrix']): On 2012/02/13 18:52:46, antonm wrote: > maybe put this information into type registry? This check is a temporary hack from old generator. I'm trying to add hacks in place rather than to type registry. Added a fixme to add proper support for non-custom constructors. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2670: self._interface.id in ['DOMWindow', 'Element', 'HTMLElement', 'SVGElement']): On 2012/02/13 18:52:46, antonm wrote: > ditto for type registry Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2731: idl_type = GetIDLTypeInfo(attr.type).idl_type() On 2012/02/13 18:52:46, antonm wrote: > again, type registry? Done. http://codereview.chromium.org/9390009/diff/1/client/dom/scripts/dartgenerato... client/dom/scripts/dartgenerator.py:2947: ' v8::HandleScope handleScope;\n' On 2012/02/13 18:52:46, antonm wrote: > separate template file? It's convenient to have small snippets inline, I think we should not use template here.
LGTM http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... File client/dom/scripts/dartgenerator.py (right): http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:532: generators = filter(lambda x: x, generators) filter(None, generators) should do to. Up to you if it's too esoteric. http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2298: if self._native_type: nit: if self._native_type is not None might be more explicit. http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2525: parameters = [] up to you, but another option would be: parameters = ', '.join(['%s %s' % (GetUDLTypeInfo(arg.type).parameter_type(), arg.id) for arg in operation.arguments()] arguments = ', '.join([arg.id for arg in operation.arguments]) http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2626: self._templates.Load('cpp_all_in_one.template'), just curious: how are you going to customize it? we eventually should support both all in one and split version. http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2747: if ('CustomToJS' in self._interface.ext_attrs or FIXME? http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2792: getter and set(['Custom', 'CustomGetter']) & set(getter.ext_attrs)): & ?
http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... File client/dom/scripts/dartgenerator.py (right): http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:532: generators = filter(lambda x: x, generators) On 2012/02/14 14:18:22, antonm wrote: > filter(None, generators) should do to. Up to you if it's too esoteric. Done. http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2626: self._templates.Load('cpp_all_in_one.template'), On 2012/02/14 14:18:22, antonm wrote: > just curious: how are you going to customize it? we eventually should support > both all in one and split version. Will add a parameter probably. http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2747: if ('CustomToJS' in self._interface.ext_attrs or On 2012/02/14 14:18:22, antonm wrote: > FIXME? Why fixme? http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2792: getter and set(['Custom', 'CustomGetter']) & set(getter.ext_attrs)): On 2012/02/14 14:18:22, antonm wrote: > & ? set intersection.
http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... File client/dom/scripts/dartgenerator.py (right): http://codereview.chromium.org/9390009/diff/2001/client/dom/scripts/dartgener... client/dom/scripts/dartgenerator.py:2747: if ('CustomToJS' in self._interface.ext_attrs or Is it possible to put attributes check into custom_to_dart method? On 2012/02/14 17:47:58, podivilov wrote: > On 2012/02/14 14:18:22, antonm wrote: > > FIXME? > > Why fixme? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
