Issue 29398655: Issue 5062 - [emscripten] Allow generation of custom bindings code (Closed)
|
Created:
March 30, 2017, 7:59 a.m. by Wladimir Palant Modified:
April 18, 2017, 11:45 a.m. CC:
Felix Dahlke Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionIssue 5062 - [emscripten] Allow generation of custom bindings code
Patch Set 1 #
Total comments: 26
Patch Set 2 : Improved initialization code #
Total comments: 4
Patch Set 3 : Made the code slightly safer #
Total comments: 2
Patch Set 4 : Improved binding generation #Patch Set 5 : Better variable name #
Total comments: 2
Patch Set 6 : Fixed mistakenly exported function #Patch Set 7 : Rebased #
MessagesTotal messages: 14
I have not tried it, but in general looks good, only a couple of moments. https://codereview.adblockplus.org/29398655/diff/29398656/README.md File README.md (left): https://codereview.adblockplus.org/29398655/diff/29398656/README.md#oldcode22 README.md:22: * [Node.js 6 or higher](https://nodejs.org/en/) I think node js is still required, why is it removed? https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:620: };\ I would make it a simple function `void InitializeBindings();`. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ In C++ there is no need for specifying of void in arguments. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ Do you mind to consider in future moving of such C++ code into cpp files? https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:625: struct BindingsInitializer instance;\ Why did you need to add struct here? https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:50: StringMap<int> typeMap { BTW, it would be better to add const here. I think in another commit. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:348: { BTW, what about having of this method under `#ifdef PRINT_BINDINGS`? JIC, it would be better to do it in another commit. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:361: type_cstr[i] = toupper(type[i]); Just wonder, does compiler generate a warning here? https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:367: } This implementation smells but since it's only to generate JS files, it's OK.
https://codereview.adblockplus.org/29398655/diff/29398656/README.md File README.md (left): https://codereview.adblockplus.org/29398655/diff/29398656/README.md#oldcode22 README.md:22: * [Node.js 6 or higher](https://nodejs.org/en/) On 2017/03/30 11:09:17, sergei wrote: > I think node js is still required, why is it removed? Not for compiling our C++ code - there we reuse Node from Emscripten now so it is covered with the Emscripten requirement. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:620: };\ On 2017/03/30 11:09:17, sergei wrote: > I would make it a simple function `void InitializeBindings();`. Done. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ On 2017/03/30 11:09:17, sergei wrote: > In C++ there is no need for specifying of void in arguments. Done. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ On 2017/03/30 11:09:17, sergei wrote: > Do you mind to consider in future moving of such C++ code into cpp files? Not sure what you mean. This code is inside bindings.cpp - but it is being generated by a macro. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:625: struct BindingsInitializer instance;\ On 2017/03/30 11:09:17, sergei wrote: > Why did you need to add struct here? This is a InitializeBindings() call now - this was implicitly running the constructor. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:348: { On 2017/03/30 11:09:18, sergei wrote: > BTW, what about having of this method under `#ifdef PRINT_BINDINGS`? Why? The compiler will remove it if not needed. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:361: type_cstr[i] = toupper(type[i]); On 2017/03/30 11:09:17, sergei wrote: > Just wonder, does compiler generate a warning here? Nope, for me it doesn't. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:367: } On 2017/03/30 11:09:17, sergei wrote: > This implementation smells but since it's only to generate JS files, it's OK. Yes, this isn't runtime code.
Added Hubert to CC.
https://codereview.adblockplus.org/29398655/diff/29398712/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398712/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:353: for (auto it = typeMap.begin(); it != typeMap.end(); ++it) Shouldn't we use a range iterator here (with const ref for the value) ? Short of that, at least a const iterator. https://codereview.adblockplus.org/29398655/diff/29398712/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:362: } Also we don't check that i or type.length() is < 255.
https://codereview.adblockplus.org/29398655/diff/29398712/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398712/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:353: for (auto it = typeMap.begin(); it != typeMap.end(); ++it) On 2017/03/31 02:57:22, hub wrote: > Shouldn't we use a range iterator here (with const ref for the value) ? > > Short of that, at least a const iterator. Done. https://codereview.adblockplus.org/29398655/diff/29398712/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:362: } On 2017/03/31 02:57:22, hub wrote: > Also we don't check that i or type.length() is < 255. I didn't really want to bother but let's do a proper size check here. No point recovering however, it's a fatal error.
https://codereview.adblockplus.org/29398655/diff/29398656/README.md File README.md (left): https://codereview.adblockplus.org/29398655/diff/29398656/README.md#oldcode22 README.md:22: * [Node.js 6 or higher](https://nodejs.org/en/) On 2017/03/30 12:58:57, Wladimir Palant wrote: > On 2017/03/30 11:09:17, sergei wrote: > > I think node js is still required, why is it removed? > > Not for compiling our C++ code - there we reuse Node from Emscripten now so it > is covered with the Emscripten requirement. Acknowledged. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ On 2017/03/30 12:58:58, Wladimir Palant wrote: > On 2017/03/30 11:09:17, sergei wrote: > > Do you mind to consider in future moving of such C++ code into cpp files? > > Not sure what you mean. This code is inside bindings.cpp - but it is being > generated by a macro. I mean it would be better to have this code in another cpp file than use preprocessor, e.g. we could simply append the file with main function to a list of cpp files. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:348: { On 2017/03/30 12:58:58, Wladimir Palant wrote: > On 2017/03/30 11:09:18, sergei wrote: > > BTW, what about having of this method under `#ifdef PRINT_BINDINGS`? > > Why? The compiler will remove it if not needed. Generally yes, but I'm not sure that compiler can always be so smart, so to avoid manual checks whether it's actually removed or not I would recommend to help the compiler by adding #ifdef, maybe even it can help us in future to identify some bugs. https://codereview.adblockplus.org/29398655/diff/29402596/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29402596/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:364: throw std::runtime_error("Value size too large for the buffer"); Well, if you touch this code then I would recommend to get rid of at least "type_cstr" variable and optionally "OwnedString type" variable, use std::string instead of "char type_cstr[256];" and move it into the scope of loop. something like this: for (const auto& typeEntry : typeMap) { std::string type(typeEntry.first.length(), '\0'); for (int i = 0; i < typeEntry.first.length(); ++i) { type[i] = typeEntry.first[i] == '-' ? '_' : toupper(typeEntry.first[i]); } printf(" %s: %i,\n", type.c_str(), it.second); }
https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ On 2017/04/04 14:49:30, sergei wrote: > I mean it would be better to have this code in another cpp file than use > preprocessor, e.g. we could simply append the file with main function to a list > of cpp files. But we are not compiling an executable, it's not supposed to have a main() function. The fact that the intermediate binding generation step is compiling an executable that we throw away is merely an implementation detail. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:348: { On 2017/04/04 14:49:30, sergei wrote: > Generally yes, but I'm not sure that compiler can always be so smart, I sincerely disagee - we are adding EMSCRIPTEN_KEEPALIVE to functions for the very reason that any unused code will be thrown away otherwise. I'd rather keep all the technical details of bindings generation (including PRINT_BINDINGS define) in bindings.ipp rather than spread it through the entire code base. https://codereview.adblockplus.org/29398655/diff/29402596/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29402596/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:364: throw std::runtime_error("Value size too large for the buffer"); On 2017/04/04 14:49:31, sergei wrote: > Well, if you touch this code then I would recommend to get rid of at least > "type_cstr" variable and optionally "OwnedString type" variable, use std::string > instead of "char type_cstr[256];" and move it into the scope of loop. > > something like this: > > for (const auto& typeEntry : typeMap) { > std::string type(typeEntry.first.length(), '\0'); > for (int i = 0; i < typeEntry.first.length(); ++i) { > type[i] = typeEntry.first[i] == '-' ? '_' : toupper(typeEntry.first[i]); > } > printf(" %s: %i,\n", type.c_str(), it.second); > } Done.
In Patch set 5 I changed the loop variable name to item which is consistent with the other range-based loops.
LGTM
LGTM https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ On 2017/04/04 15:41:48, Wladimir Palant wrote: > On 2017/04/04 14:49:30, sergei wrote: > > I mean it would be better to have this code in another cpp file than use > > preprocessor, e.g. we could simply append the file with main function to a > list > > of cpp files. > > But we are not compiling an executable, it's not supposed to have a main() > function. The fact that the intermediate binding generation step is compiling an > executable that we throw away is merely an implementation detail. I didn't mean to append a cpp with main to SOURCE_FILES which is also used in run_compiler, I meant it could be appended only to the list of cpp files used in generate_bindings. Even more, it's not only getting rid of defines with the code but also getting it clearer and less error-prone. Clearer because it would be clear which code is used only for generator, which is used only for regular run and which is a common set. Less error-prone because we would eliminate even an accidental possibility of influence from the generator code on a regular run code. For instance only for generator we need non-existing currently main.cpp, bindings.cpp and bindings.ipp, however we use two latter in compilation of JS code which is for regular run. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:348: { On 2017/04/04 15:41:48, Wladimir Palant wrote: > On 2017/04/04 14:49:30, sergei wrote: > > Generally yes, but I'm not sure that compiler can always be so smart, > > I sincerely disagee - we are adding EMSCRIPTEN_KEEPALIVE to functions for the > very reason that any unused code will be thrown away otherwise. I'm pretty sure that compiler is not removing this method because the mangled name of it is present in lib/compiled.js, there is some obfuscated function associated with it which can be an implementation of it and after adding of #ifdef PRINT_BINDINGS void RegExpFilter::GenerateCustomBindings() {...} #endif it has gone and the size of lib/compiled.js decreased by ~4KB. I think that we are adding EMSCRIPTEN_KEEPALIVE to prevent a method from removing when compiler thinks that it's not used but it does not mean that the compiler is able to find absolutely all unused methods. > > I'd rather keep all the technical details of bindings generation (including > PRINT_BINDINGS define) in bindings.ipp rather than spread it through the entire > code base. It's actually related to my comment regarding separate C++ files (error-prone part), we could move such methods into cpp files which are used only for generation, though sometimes it can require some additional efforts, like in this case where access to typeMap should be reconsidered. https://codereview.adblockplus.org/29398655/diff/29404583/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29404583/compiled/bindings.i... compiled/bindings.ipp:319: std::vector<CustomGenerator> customGenerators; These global variables can also contribute into lib/compiled.js and it would be better to use them only for generator.
Patch set 6 removes bogus EMSCRIPTEN_KEEPALIVE. Other than that it is only rebasing changes. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/bindings.i... compiled/bindings.ipp:621: int main(void)\ On 2017/04/11 16:29:20, sergei wrote: > I didn't mean to append a cpp with main to SOURCE_FILES which is also used in > run_compiler, I meant it could be appended only to the list of cpp files used in > generate_bindings. > > Even more, it's not only getting rid of defines with the code but also getting > it clearer and less error-prone. Clearer because it would be clear which code is > used only for generator, which is used only for regular run and which is a > common set. Less error-prone because we would eliminate even an accidental > possibility of influence from the generator code on a regular run code. > > For instance only for generator we need non-existing currently main.cpp, > bindings.cpp and bindings.ipp, however we use two latter in compilation of JS > code which is for regular run. Let's do this in a follow-up, I'll file an issue. https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... File compiled/filter/RegExpFilter.cpp (right): https://codereview.adblockplus.org/29398655/diff/29398656/compiled/filter/Reg... compiled/filter/RegExpFilter.cpp:348: { On 2017/04/11 16:29:20, sergei wrote: > I'm pretty sure that compiler is not removing this method because the mangled > name of it is present in lib/compiled.js, That's actually me doing something stupid - this function is declared with EMSCRIPTEN_KEEPALIVE flag. Fixed. > It's actually related to my comment regarding separate C++ files (error-prone > part), we could move such methods into cpp files which are used only for > generation, though sometimes it can require some additional efforts, like in > this case where access to typeMap should be reconsidered. Not sure I like this idea, code locality being important for maintainability. In fact, I might even prefer declaring bindings in the respective class files rather than in a central place. https://codereview.adblockplus.org/29398655/diff/29404583/compiled/bindings.ipp File compiled/bindings.ipp (right): https://codereview.adblockplus.org/29398655/diff/29404583/compiled/bindings.i... compiled/bindings.ipp:319: std::vector<CustomGenerator> customGenerators; On 2017/04/11 16:29:20, sergei wrote: > These global variables can also contribute into lib/compiled.js and it would be > better to use them only for generator. Ok, I can see your point. Yes, they are currently being added to the final build and there doesn't seem to be an easy way to get rid of them.
On 2017/04/11 18:22:44, Wladimir Palant wrote: > Patch set 6 removes bogus EMSCRIPTEN_KEEPALIVE. Other than that it is only > rebasing changes. LGTM
Message was sent while issue was closed.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
