Issue 5131 - [emscripten] Clean separation of bindings code and runtime code

Issue 5131 - [emscripten] Clean separation of bindings code and runtime code

[Wladimir Palant, 2017-04-12 14:17:00]

https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/g...
compiled/bindings/generator.cpp:24: std::vector<bindings_internal::ClassInfo>
classes;
I found two bad assumptions here which went wrong after this refactoring.

1) This was originally a map and I assumed that iterating over it I would get
the members in the order in which they were added. They are actually sorted by
type ID which isn't necessarily incrementing in the order we want. So this is a
vector now, searching it isn't an impact on performance we care about for
bindings generation.

2) ClassInfo::baseClass member was a pointer referring to an instance within
this map. This was a bad idea as pointers to map/vector buffer aren't guaranteed
to stay stable as items are added. So ClassInfo::baseClass is a TYPEID now and
looked up when needed.

https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/g...
File compiled/bindings/generator.h (right):

https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/g...
compiled/bindings/generator.h:225: class_(const char* name)
I would have preferred to move the implementations here into generator.cpp as
well but that appears to be still impossible with the current version of C++
standard.

[Wladimir Palant, 2017-04-18 09:07:34]

Actually, if we call printBindings() explicitly then we don't need a special
mechanism for custom generators - we can simply call these functions at the end
of main(). This is what Patch Set 3 does now.

[sergei, 2017-04-18 10:46:09]

Could you please rebase it?

https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/g...
compiled/bindings/generator.cpp:24: std::vector<bindings_internal::ClassInfo>
classes;
On 2017/04/12 14:17:00, Wladimir Palant wrote:
> I found two bad assumptions here which went wrong after this refactoring.
> 
> 1) This was originally a map and I assumed that iterating over it I would get
> the members in the order in which they were added. They are actually sorted by
> type ID which isn't necessarily incrementing in the order we want. So this is
a
> vector now, searching it isn't an impact on performance we care about for
> bindings generation.
> 
> 2) ClassInfo::baseClass member was a pointer referring to an instance within
> this map. This was a bad idea as pointers to map/vector buffer aren't
guaranteed
> to stay stable as items are added. So ClassInfo::baseClass is a TYPEID now and
> looked up when needed.

That's correct, pointers and iterators are invalidated by almost all operations
modifying std::map and vector. One can use iterators of std::list but here I
think having TYPEID value is even better.

https://codereview.adblockplus.org/29410617/diff/29416555/compile
File compile (right):

https://codereview.adblockplus.org/29410617/diff/29416555/compile#newcode55
compile:55: not f.startswith('runtime_')
I expected bindings/runtime_utils.cpp and files for generator to be in different
directories.

[Wladimir Palant, 2017-04-18 11:01:37]

On 2017/04/18 10:46:09, sergei wrote:
> Could you please rebase it?

It is rebased already, this change applies on top of
https://codereview.adblockplus.org/29398669/ however.

https://codereview.adblockplus.org/29410617/diff/29416555/compile
File compile (right):

https://codereview.adblockplus.org/29410617/diff/29416555/compile#newcode55
compile:55: not f.startswith('runtime_')
On 2017/04/18 10:46:09, sergei wrote:
> I expected bindings/runtime_utils.cpp and files for generator to be in
different
> directories.

I know. However, IMHO code locality is more important here - this is all the
code related to JS bindings, including the runtime parts that the generator
expects. If anything, maybe it should be bindings/generation/{main|generator}
and bindings/runtime/utils.

[sergei, 2017-04-20 12:13:39]

On 2017/04/18 11:01:37, Wladimir Palant wrote:
> On 2017/04/18 10:46:09, sergei wrote:
> > Could you please rebase it?
> 
> It is rebased already, this change applies on top of
> https://codereview.adblockplus.org/29398669/ however.
The latter is landed but the patch still cannot be applied.

[Wladimir Palant, 2017-04-20 13:50:47]

On 2017/04/20 12:13:39, sergei wrote:
> > It is rebased already, this change applies on top of
> > https://codereview.adblockplus.org/29398669/ however.
> The latter is landed but the patch still cannot be applied.

Ok, Patch Set 4 has been rebased again. There were some minor compile script
changes in between, not really something that would prevent the patch from
applying however.

[sergei, 2017-04-20 14:52:00]

LGTM

https://codereview.adblockplus.org/29410617/diff/29418647/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29410617/diff/29418647/compiled/bindings/m...
compiled/bindings/main.cpp:42: class_<Filter>("Filter")
Maybe it worth to leave these code in a function and call the latter from main.
The current version is is fine me, though.

[Wladimir Palant, 2017-04-20 14:54:29]

https://codereview.adblockplus.org/29410617/diff/29418647/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29410617/diff/29418647/compiled/bindings/m...
compiled/bindings/main.cpp:42: class_<Filter>("Filter")
On 2017/04/20 14:52:00, sergei wrote:
> Maybe it worth to leave these code in a function and call the latter from
main.
> The current version is is fine me, though.

This is worth considering but let's land this change first - I really need to
get it out of my patch queue :)