Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(24)

Issue 29410617: Issue 5131 - [emscripten] Clean separation of bindings code and runtime code (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by Wladimir Palant
Modified:
2 years, 7 months ago
Reviewers:
sergei
CC:
hub, Felix Dahlke
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rebased #

Patch Set 3 : Call custom generators explicitly #

Total comments: 2

Patch Set 4 : Rebased #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+363 lines, -1114 lines) Patch
M compile View 1 2 3 3 chunks +15 lines, -8 lines 0 comments Download
M compiled/bindings/generator.h View 1 2 5 chunks +20 lines, -522 lines 0 comments Download
M compiled/bindings/generator.cpp View 1 2 4 chunks +167 lines, -478 lines 0 comments Download
M compiled/bindings/main.cpp View 1 2 1 chunk +122 lines, -106 lines 2 comments Download
A compiled/bindings/runtime_utils.cpp View 1 chunk +39 lines, -0 lines 0 comments Download

Messages

Total messages: 9
Wladimir Palant
2 years, 7 months ago (2017-04-12 14:07:26 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/generator.cpp File compiled/bindings/generator.cpp (right): https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/generator.cpp#newcode24 compiled/bindings/generator.cpp:24: std::vector<bindings_internal::ClassInfo> classes; I found two bad assumptions here which ...
2 years, 7 months ago (2017-04-12 14:17:00 UTC) #2
Wladimir Palant
Actually, if we call printBindings() explicitly then we don't need a special mechanism for custom ...
2 years, 7 months ago (2017-04-18 09:07:34 UTC) #3
sergei
Could you please rebase it? https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/generator.cpp File compiled/bindings/generator.cpp (right): https://codereview.adblockplus.org/29410617/diff/29410618/compiled/bindings/generator.cpp#newcode24 compiled/bindings/generator.cpp:24: std::vector<bindings_internal::ClassInfo> classes; On 2017/04/12 ...
2 years, 7 months ago (2017-04-18 10:46:09 UTC) #4
Wladimir Palant
On 2017/04/18 10:46:09, sergei wrote: > Could you please rebase it? It is rebased already, ...
2 years, 7 months ago (2017-04-18 11:01:37 UTC) #5
sergei
On 2017/04/18 11:01:37, Wladimir Palant wrote: > On 2017/04/18 10:46:09, sergei wrote: > > Could ...
2 years, 7 months ago (2017-04-20 12:13:39 UTC) #6
Wladimir Palant
On 2017/04/20 12:13:39, sergei wrote: > > It is rebased already, this change applies on ...
2 years, 7 months ago (2017-04-20 13:50:47 UTC) #7
sergei
LGTM https://codereview.adblockplus.org/29410617/diff/29418647/compiled/bindings/main.cpp File compiled/bindings/main.cpp (right): https://codereview.adblockplus.org/29410617/diff/29418647/compiled/bindings/main.cpp#newcode42 compiled/bindings/main.cpp:42: class_<Filter>("Filter") Maybe it worth to leave these code ...
2 years, 7 months ago (2017-04-20 14:52:00 UTC) #8
Wladimir Palant
2 years, 7 months ago (2017-04-20 14:54:29 UTC) #9
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 :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5