Draft implementation of c++ bindings generation.

Draft implementation of c++ bindings generation.

Generate stubs for all c++ files.

R=antonm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=4133

[antonm, 2012-02-03 14:03:06]

Pretty neat!

1st round of comments.

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2100: cpp_impl_path =
os.path.join(self._output_dir, 'cpp',
nit: maybe introduce a helper a la
_FilePathForDartInterface/_FilePathForDartImplementation and for C++ files?

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2143: # Generate DartDerivedSourcesAll.cpp
nit: missing trailing dot.

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2149: path = os.path.relpath(file,
os.path.dirname(cpp_all_in_one_path))
nit: two-space indent for control flow.

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2150: includes_emitter.Emit('#include
"$PATH"\n', PATH=path)
what about ENABLE_ features?  Do you have filters before?

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2155: INCLUDES=includes_emitter.Fragments())
just curious, I don't know if the templating system supports it: in many other
template systems, I'd rather have a loop to generate includes in template itself
and pass only a list of files to include into a template.  Something like:

%for $include in $INCLUDES
#include "$include"
%endfor

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2162: for file in self._cpp_header_files:
file is a synonym for open in Python and it's usually recommended not to use it.

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2166: '    if (Dart_NativeFunction func =
$CLASS_NAME::resolver(name, argumentCount))\n'
multiline literal?

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2168:
CLASS_NAME=os.path.splitext(os.path.basename(path))[0])
hrr, do not we have it stored somewhere already?

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/templates/do...
File client/dom/templates/dom/native/cpp_callback_header.template (right):

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/templates/do...
client/dom/templates/dom/native/cpp_callback_header.template:8: #define
Dart$(INTERFACE)_h
ENABLEs?

[sra1, 2012-02-03 21:30:49]

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
File client/dom/scripts/dartgenerator.py (right):

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2154:
self._templates.Load('cpp_all_in_one.template'),
Instead of using Fragments to build and substitute a substring, use the 'holes'
feature to write directly to a substring.

The above ~10 lines would be done 'top-down' instead of 'bottom-up'

1. Change template to have a hole: $!INCLUDES

2. Work top-down:
cpp_all_in_one_emitter = ...

# Emit template and collect sub-emitters
includes_emitter = cpp_all_in_one_emitter.Emit(
  self._templates.Load('cpp_all_in_one.template'))

# Use sub-emitters
for filename in self._cpp_impl_files:
  path = ...
  includes_emitter.Emit('#include ...', PATH=...)

There is a comment in emitter.py that might add more detail about the mechanism.

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2155: INCLUDES=includes_emitter.Fragments())
On 2012/02/03 14:03:06, antonm wrote:
> just curious, I don't know if the templating system supports it: in many other
> template systems, I'd rather have a loop to generate includes in template
itself
> and pass only a list of files to include into a template.  Something like:
> 
> %for $include in $INCLUDES
> #include "$include"
> %endfor

Lets spend a few minutes talking about the templates in our next meeting.  It
currently does not support control flow.
I don't really want to add control flow, since that requires building a lot of
machinery.

https://chromiumcodereview.appspot.com/9316096/diff/1/client/dom/scripts/dart...
client/dom/scripts/dartgenerator.py:2174:
RESOLVER_BODY=resolver_body_emitter.Fragments())
See above comment about using 'holes'.

Start resolver code with:

(includes_emitter, resolver_body_emitter) = cpp_resolver_emitter.Emit(
   self._templates.Load('cpp_resolver.template'))

Python gives you a error if the number of holes does not match the tuple length.

[podivilov, 2012-02-06 13:24:36]

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2100: cpp_impl_path =
os.path.join(self._output_dir, 'cpp',
On 2012/02/03 14:03:06, antonm wrote:
> nit: maybe introduce a helper a la
> _FilePathForDartInterface/_FilePathForDartImplementation and for C++ files?

Done.

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2143: # Generate DartDerivedSourcesAll.cpp
On 2012/02/03 14:03:06, antonm wrote:
> nit: missing trailing dot.

Done.

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2149: path = os.path.relpath(file,
os.path.dirname(cpp_all_in_one_path))
On 2012/02/03 14:03:06, antonm wrote:
> nit: two-space indent for control flow.

Done.

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2150: includes_emitter.Emit('#include
"$PATH"\n', PATH=path)
On 2012/02/03 14:03:06, antonm wrote:
> what about ENABLE_ features?  Do you have filters before?

Disabled interfaces and operations are removed by fremontcutbuilder.py

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2154:
self._templates.Load('cpp_all_in_one.template'),
On 2012/02/03 21:30:49, sra1 wrote:
> Instead of using Fragments to build and substitute a substring, use the
'holes'
> feature to write directly to a substring.
> 
> The above ~10 lines would be done 'top-down' instead of 'bottom-up'
> 
> 1. Change template to have a hole: $!INCLUDES
> 
> 2. Work top-down:
> cpp_all_in_one_emitter = ...
> 
> # Emit template and collect sub-emitters
> includes_emitter = cpp_all_in_one_emitter.Emit(
>   self._templates.Load('cpp_all_in_one.template'))
> 
> # Use sub-emitters
> for filename in self._cpp_impl_files:
>   path = ...
>   includes_emitter.Emit('#include ...', PATH=...)
> 
> There is a comment in emitter.py that might add more detail about the
mechanism.

Not using template holes was a conscious decision actually. Consider generating
a list of c++ inclusions: you need to maintain a unique list of files while
generating operations, then emit it in lex order. When going top-down, you are
obliged to create a useless hole and emitter for it. Also, I think the code is
easier to understand and maintain when it uses only one type of template
parameters instead of two.

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2162: for file in self._cpp_header_files:
On 2012/02/03 14:03:06, antonm wrote:
> file is a synonym for open in Python and it's usually recommended not to use
it.

Done.

http://codereview.chromium.org/9316096/diff/1/client/dom/scripts/dartgenerato...
client/dom/scripts/dartgenerator.py:2166: '    if (Dart_NativeFunction func =
$CLASS_NAME::resolver(name, argumentCount))\n'
On 2012/02/03 14:03:06, antonm wrote:
> multiline literal?

Multiline literals break indentation rules and make the code hard to read. I
suggest that we stay with single line strings for code generation.

http://codereview.chromium.org/9316096/diff/1/client/dom/templates/dom/native...
File client/dom/templates/dom/native/cpp_callback_header.template (right):

http://codereview.chromium.org/9316096/diff/1/client/dom/templates/dom/native...
client/dom/templates/dom/native/cpp_callback_header.template:8: #define
Dart$(INTERFACE)_h
On 2012/02/03 14:03:06, antonm wrote:
> ENABLEs?

They are not needed anymore as databasebuilder supports Conditional attributes.

[antonm, 2012-02-06 13:45:51]

LGTM

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2195:
CLASS_NAME=os.path.splitext(os.path.basename(path))[0])
up to you, but I find those basename/splitext games somewhat confusing.  Do not
you have a list of interfaces at hand?

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2227: """Generates Dart and c++ code for the
given interface.
nit: C++

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2238: cpp_header_emitter: an Emitter for the
file containing the c++ header.
again (here and below), C++ imho is preferred.

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
File client/dom/templates/dom/native/cpp_callback_header.template (right):

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
client/dom/templates/dom/native/cpp_callback_header.template:39: #endif //
Dart$(INTERFACE)_h
nit: do we need this trailing // Dart$(INTERFACE) ?

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
File client/dom/templates/dom/native/cpp_header.template (right):

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
client/dom/templates/dom/native/cpp_header.template:46: #endif //
Dart$(INTERFACE)_h
ditto

[podivilov, 2012-02-10 15:59:37]

Thanks!

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
File client/dom/scripts/dartgenerator.py (right):

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2195:
CLASS_NAME=os.path.splitext(os.path.basename(path))[0])
On 2012/02/06 13:45:51, antonm wrote:
> up to you, but I find those basename/splitext games somewhat confusing.  Do
not
> you have a list of interfaces at hand?

not really...

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2227: """Generates Dart and c++ code for the
given interface.
On 2012/02/06 13:45:51, antonm wrote:
> nit: C++

Done.

http://codereview.chromium.org/9316096/diff/5001/client/dom/scripts/dartgener...
client/dom/scripts/dartgenerator.py:2238: cpp_header_emitter: an Emitter for the
file containing the c++ header.
On 2012/02/06 13:45:51, antonm wrote:
> again (here and below), C++ imho is preferred.

Done.

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
File client/dom/templates/dom/native/cpp_callback_header.template (right):

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
client/dom/templates/dom/native/cpp_callback_header.template:39: #endif //
Dart$(INTERFACE)_h
On 2012/02/06 13:45:51, antonm wrote:
> nit: do we need this trailing // Dart$(INTERFACE) ?

Yes, most webcore headers have them. Old bindings also had them. Why do you
think we should remove them from here?

[antonm, 2012-02-10 16:01:06]

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
File client/dom/templates/dom/native/cpp_callback_header.template (right):

http://codereview.chromium.org/9316096/diff/5001/client/dom/templates/dom/nat...
client/dom/templates/dom/native/cpp_callback_header.template:39: #endif //
Dart$(INTERFACE)_h
Some time ago I was told by ap that they are not required any more.  Things may
have changed again since then.  Anyway, cosmetic, feel free to ignore.

On 2012/02/10 15:59:37, podivilov wrote:
> On 2012/02/06 13:45:51, antonm wrote:
> > nit: do we need this trailing // Dart$(INTERFACE) ?
> 
> Yes, most webcore headers have them. Old bindings also had them. Why do you
> think we should remove them from here?