Issue 5062 - [emscripten] Allow generation of custom bindings code

compiled/filter/RegExpFilter.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <cctype>
 #include <climits>
+#include <cstdio>
 
 #include <emscripten.h>
 
 #include "RegExpFilter.h"
 #include "../StringScanner.h"
 #include "../StringMap.h"
 
 namespace
 {
   enum
   {
     TYPE_OTHER = 0x1,
     TYPE_SCRIPT = 0x2,
     TYPE_IMAGE = 0x4,
     TYPE_STYLESHEET = 0x8,
     TYPE_OBJECT = 0x10,
     TYPE_SUBDOCUMENT = 0x20,
     TYPE_DOCUMENT = 0x40,
     TYPE_PING = 0x400,
     TYPE_XMLHTTPREQUEST = 0x800,
     TYPE_OBJECT_SUBREQUEST = 0x1000,
     TYPE_MEDIA = 0x4000,
     TYPE_FONT = 0x8000,
     TYPE_POPUP = 0x8000000,
     TYPE_GENERICBLOCK = 0x10000000,
     TYPE_GENERICHIDE = 0x20000000,
     TYPE_ELEMHIDE = 0x40000000,
   };
 
   StringMap<int> typeMap {

[sergei, 2017/03/30 11:09:17]

BTW, it would be better to add const here. I think in another commit.

     {u"other"_str, TYPE_OTHER},
     {u"script"_str, TYPE_SCRIPT},
     {u"image"_str, TYPE_IMAGE},
     {u"stylesheet"_str, TYPE_STYLESHEET},
     {u"object"_str, TYPE_OBJECT},
     {u"subdocument"_str, TYPE_SUBDOCUMENT},
     {u"document"_str, TYPE_DOCUMENT},
     {u"xbl"_str, TYPE_OTHER},          // Backwards compat
     {u"ping"_str, TYPE_PING},
     {u"xmlhttprequest"_str, TYPE_XMLHTTPREQUEST},
@@ skipping 276 matching lines @@
     done = scanner.done();
     if (scanner.next() == u'|')
     {
       if (scanner.position() > start)
         AddSitekey(DependentString(sitekeys, start, scanner.position() - start));
       start = scanner.position() + 1;
     }
   }
 }
 
-void RegExpFilter::InitJSTypes()
+void RegExpFilter::GenerateCustomBindings()
 {

[sergei, 2017/03/30 11:09:18]

BTW, what about having of this method under `#ifdef PRINT_BINDINGS`? JIC, it
would be better to do it in another commit.

[Wladimir Palant, 2017/03/30 12:58:58]

Why? The compiler will remove it if not needed.

[sergei, 2017/04/04 14:49:30]

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.

[Wladimir Palant, 2017/04/04 15:41:48]

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.

[sergei, 2017/04/11 16:29:20]

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.
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.

[Wladimir Palant, 2017/04/11 18:22:44]

That's actually me doing something stupid - this function is declared with
EMSCRIPTEN_KEEPALIVE flag. Fixed.
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.

-  EM_ASM(exports.RegExpFilter.typeMap = {};);
+  printf("exports.RegExpFilter.typeMap = {\n");
+
+  OwnedString type;
+  char type_cstr[256];
   for (auto it = typeMap.begin(); it != typeMap.end(); ++it)
-    EM_ASM_ARGS(exports.RegExpFilter.typeMap[readString($0).replace("-", "_").toUpperCase()] = $1, &(it->first), it->second);
+  {
+    type = it->first;
+    for (int i = 0; i < type.length(); i++)
+    {
+      if (type[i] == '-')
+        type_cstr[i] = '_';
+      else
+        type_cstr[i] = toupper(type[i]);

[sergei, 2017/03/30 11:09:17]

Just wonder, does compiler generate a warning here?

[Wladimir Palant, 2017/03/30 12:58:58]

Nope, for me it doesn't.

+    }
+    type_cstr[type.length()] = 0;
+    printf("  %s: %i,\n", type_cstr, it->second);
+  }
+  printf("};\n");
 }

[sergei, 2017/03/30 11:09:17]

This implementation smells but since it's only to generate JS files, it's OK.

[Wladimir Palant, 2017/03/30 12:58:58]

Yes, this isn't runtime code.

 
 RegExpFilter::DomainMap* RegExpFilter::GetDomains() const
 {
   if (!mData.DomainsParsingDone())
   {
     ParseDomains(mData.GetDomainsSource(mText), u'|');
     mData.SetDomainsParsingDone();
   }
   return ActiveFilter::GetDomains();
 }
@@ skipping 19 matching lines @@
     return false;
   }
 
   if (!mData.RegExpParsingDone())
   {
     const OwnedString pattern(mData.GetRegExpSource(mText));
     mData.SetRegExp(GenerateRegExp(RegExpFromSource(pattern), mData.mMatchCase));
   }
   return EM_ASM_INT(return regexps.test($0, $1), mData.mRegexpId, &location);
 }