Issue 4125 - [emscripten] Convert filter classes to C++

The implementation is still hacky in many places, these can probably be improved. You can clone the code from https://bitbucket.org/palant/adblockpluscore/commits/branch/emscripten if you want to play with it.

[Felix Dahlke, 2016-01-15 10:29:18]

So you didn't have to convert subscriptionClasses after all :)

It's rough around the edges, I could leave a bunch of comments. But I think the
better course of action is to keep things hacky and also rewrite
subscriptionClasses, then do some tests to see the effects. I don't think
there's much point in a proper review unless we know the benefits.

[Wladimir Palant, 2016-01-15 12:52:09]

On 2016/01/15 10:29:18, Felix Dahlke wrote:
> So you didn't have to convert subscriptionClasses after all :)

Of course I do - that's still very incomplete, and Filter.subscriptions for
example is marked as TODO. What I have here is merely enough to pass the few
tests I added to test/filterClasses.js.

> It's rough around the edges, I could leave a bunch of comments. But I think
the
> better course of action is to keep things hacky and also rewrite
> subscriptionClasses, then do some tests to see the effects. I don't think
> there's much point in a proper review unless we know the benefits.

No, not proper review of course. However, you might want to comment on my C++ -
there is probably some stuff worth correcting now rather than later.

[Felix Dahlke, 2016-01-15 17:00:36]

On 2016/01/15 12:52:09, Wladimir Palant wrote:
> On 2016/01/15 10:29:18, Felix Dahlke wrote:
> > So you didn't have to convert subscriptionClasses after all :)
> 
> Of course I do - that's still very incomplete, and Filter.subscriptions for
> example is marked as TODO. What I have here is merely enough to pass the few
> tests I added to test/filterClasses.js.

Should have read the tests :D

> > It's rough around the edges, I could leave a bunch of comments. But I think
> the
> > better course of action is to keep things hacky and also rewrite
> > subscriptionClasses, then do some tests to see the effects. I don't think
> > there's much point in a proper review unless we know the benefits.
> 
> No, not proper review of course. However, you might want to comment on my C++
-
> there is probably some stuff worth correcting now rather than later.

Sure, here are some comments. Note that this is just from a quick look at
things, I'm planning to take a much closer look later.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:9: extern "C"
I'm not really a fan of this approach. Have you tried embind [1]? If that works
out for us, we won't need to build this C wrapper.

If we have to build the C wrapper, I would prefer to move it out of the classes
and into a separate file. That's IIRC the common way to do it.

[1]:
https://kripken.github.io/emscripten-site/docs/porting/connecting_cpp_and_jav...

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.h:1: #ifndef ADBLOCKPLUS_ACTIVEFILTER_H
Nit: Words in camel cased names should be separated by an underscore in include
guards, i.e. `ADBLOCK_PLUS_ACTIVE_FILTER`.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.h:12: ActiveFilter(const std::u16string& text);
I'd prefer to go for std::wstring, we also use it in libadblockplus to store JS
strings, and the Emscripten tests all use std::wstring.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.h:12: ActiveFilter(const std::u16string& text);
Please use the explicit keyword for single parameter constructors - unless you
actually want the automatic conversion.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/CommentFil...
File compiled/CommentFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/CommentFil...
compiled/CommentFilter.cpp:19: if (type == Type::UNKNOWN)
IMHO it'd make more sense to check for type != Type::COMMENT here.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:6: class Scanner
Please wrap global definitions in an unnamed namespace.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
File compiled/ElemHideBase.h (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
compiled/ElemHideBase.h:12: ElemHideBase(const std::u16string& text, size_t
domainsEnd, size_t selectorStart);
I'd prefer to largely stick to the 80 column rule in new code.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/Filter.cpp...
compiled/Filter.cpp:60: Filter* EMSCRIPTEN_KEEPALIVE Filter_FromText(char16_t*
textPtr, int textLen)
If this stays (see my comments on the C wrappers above), I would much prefer to
have a regular Filter::FromText member function and just call that one from
here, likewise below.

[Wladimir Palant, 2016-01-15 20:36:16]

I managed to get embind work. The good news: there is almost no boilerplate on
our end any more, filterClassesNew.js does little beyond importing the classes.
More good news: the tests didn't have to be adjusted beyond adding
filter.delete() calls, the API didn't have to change.

Now the bad news: using embind doubled the file size. Also, we are stuck with
std::wstring (4 byte chars) because embind doesn't support anything else. And as
a minor annoyance, we can no longer intercept C++ exceptions - with embind we
can no longer wrap the calls, and emscripten itself doesn't do anything
meaningful with these exceptions. So we have to be very certain not to throw.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.h:1: #ifndef ADBLOCKPLUS_ACTIVEFILTER_H
On 2016/01/15 17:00:35, Felix Dahlke wrote:
> Nit: Words in camel cased names should be separated by an underscore in
include
> guards, i.e. `ADBLOCK_PLUS_ACTIVE_FILTER`.

Done.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.h:12: ActiveFilter(const std::u16string& text);
On 2016/01/15 17:00:35, Felix Dahlke wrote:
> Please use the explicit keyword for single parameter constructors - unless you
> actually want the automatic conversion.

Done.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ActiveFilt...
compiled/ActiveFilter.h:12: ActiveFilter(const std::u16string& text);
On 2016/01/15 17:00:35, Felix Dahlke wrote:
> I'd prefer to go for std::wstring, we also use it in libadblockplus to store
JS
> strings, and the Emscripten tests all use std::wstring.

Done though merely as a side-effect of using embind. As explained on IRC, using
char16_t is preferable because unlike wchar_t it is a fixed-width type and it's
also saves us some memory. Well, embind doesn't support std::u16string and it
crashes if size of wchar_t isn't 4 bytes.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/CommentFil...
File compiled/CommentFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/CommentFil...
compiled/CommentFilter.cpp:19: if (type == Type::UNKNOWN)
On 2016/01/15 17:00:35, Felix Dahlke wrote:
> IMHO it'd make more sense to check for type != Type::COMMENT here.

Done.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:6: class Scanner
On 2016/01/15 17:00:36, Felix Dahlke wrote:
> Please wrap global definitions in an unnamed namespace.

Done.

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
File compiled/ElemHideBase.h (right):

https://codereview.adblockplus.org/29333474/diff/29333659/compiled/ElemHideBa...
compiled/ElemHideBase.h:12: ElemHideBase(const std::u16string& text, size_t
domainsEnd, size_t selectorStart);
On 2016/01/15 17:00:36, Felix Dahlke wrote:
> I'd prefer to largely stick to the 80 column rule in new code.

Done.

[Wladimir Palant, 2016-01-16 14:07:37]

I realized that we need to specify NO_DYNAMIC_EXECUTION=1 flag, otherwise we
won't be able to use this in Chrome. The drawback: embind can no longer be used,
it relies on being able to generate code dynamically. So WebIDL it is, maybe
this will work for us - otherwise back to manual approach.

[Wladimir Palant, 2016-01-16 19:28:36]

As to WebIDL: only C-style strings are supported. This really means char*, no
Unicode support whatsoever. And null-terminated of course. So we have to go back
to our very custom solution.

[Wladimir Palant, 2016-01-18 12:50:08]

Patch Set 4 is back to forwarding API parameters manually. On the bright side,
our API is no longer passing raw pointers around, I figured out how we can
instantiate std::string on stack from JS code. Also, I found a set of Emscripten
parameters that brought down the code size quite considerably, currently at 175
kB.

RegExpFilter now actually does some basic parsing of the filter and instantiates
regular expressions if a raw regular expression is given. So
RegExpFilter.match() somewhat works now. This is merely proof-of-concept of
course, showing how we would access browser objects.

[Wladimir Palant, 2016-01-20 12:11:24]

Patch Set 6 is almost complete now, merely CSS property filters are missing.
Simplified element hiding syntax isn't supported - it has been deprecated a
while ago, this should be a good time to remove it. Other than that, there are a
few minor differences in parsing (affecting only irrelevant edge cases I think),
Filter.subscriptions is unimplemented and domain uppercasing is only affecting
ASCII characters (we need to do proper Unicode uppercasing here, which requires
calling into JavaScript it seems).

[Wladimir Palant, 2016-01-28 14:43:16]

Patch Set 9 has much better performance. It's using our own String class instead
of std::string which allows reusing the same memory buffer. Currently the
disadvantage is that we have to track "manually" whether that buffer is still
valid - I'd like to offload at least some of that work to the compiler. Also,
the hash tables for ActiveFilter.mDomains and ActiveFilter.mSitekeys are
allocated dynamically as needed - we were wasting memory here originally by
allocating them unconditionally.

I created my own StringMap and StringSet implementations. The STL
implementations were wasting way too much memory. I also tried Google's
sparsehash which needed much less memory - but it also produced a massive number
of allocations which made this implementation extremely slow. My very
straightforward implementation of a closed hash with quadratic probing turned
out quite fast, and the total memory used is even a bit better than with
sparsehash.

I also realized that -Oz compilation parameter optimized for code size and
degrades performance, using -O3 doesn't result in much more code but
significantly better performance.

And I am generating the JS part of the API via script (various macros in api.cpp
produce messages that are processed by compile script and turned into api.js).
The original approach with various nested functions had quite some negative
impact on performance.

[Wladimir Palant, 2016-01-28 14:53:05]

I forgot something else worth noting: constructors of various filter classes
might modify the filter text now. In particular, filter option names and domains
will be lowercased. This behavior is actually undesired, as the check for known
filters in Filter.fromText() might go against a filter text which is not final.
This would need to be addressed eventually, e.g. by merging filter parsing and
normalization and running the latter unconditionally rather than explicitly
(meaning an API change, something that I tried to avoid so far).

[Wladimir Palant, 2016-01-28 19:57:11]

Patch Set 11 got rid of std::shared_ptr which I identified as a memory waster
(requires 16 bytes per object plus 8 bytes for each pointer). This change indeed
saved us 2.5 MB, and for the first time my test shows Emscripten slightly faster
than our old code. On Firefox it is still slower but only by 10%.

[Wladimir Palant, 2016-01-28 21:32:06]

Guess what was taking up the most time? No, not the string processing - bringing
the call parameters in shape apparently, despite being already minimal with the
latest changes. So the JS wrappers are now generated with 10 static parameters
(the script currently doesn't know the number of parameters for methods), and
these parameters are passed to the exported C++ functions with the necessary
modifications. Not accessing arguments array in JavaScript any more and not
using Function.apply(). This cut down execution time by 25% in Node and even
more in Firefox.

[Wladimir Palant, 2016-02-01 21:21:01]

In Patch Set 13 I moved the generation of JS bindings from Python into C++ code.
The impact on generated code is minimal - it got slightly faster and slightly
smaller again. The C wrappers for C++ functions we had in api.cpp before are no
longer necessary however, and neither the awkward macros requiring the
parameters to be duplicated. The external API is specified in bindings.cpp very
similarly to how Embind does it, types are automatically deduced from the
functions passed in. Figuring out the mangled names of the functions requires a
huge hack but it works.

[René Jeschke, 2016-02-02 11:11:26]

Just some nits after having a quick look.

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:28: loop:
Nit: this can be removed

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/StringMap....
compiled/StringMap.h:11: template<class Entry>
Nit: The usage of `class` for declaring template parameters is discouraged,
`typename` should be used instead. There are only very few special cases where
`class` should/must be used.
See: http://blogs.msdn.com/b/slippman/archive/2004/08/11/212768.aspx

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/intrusive_...
compiled/intrusive_ptr.h:24: if (--mRefCount == 0)
Nit: having a check on `0` here feels wrong and may lead to unwanted behaviour
if one tries to release a `0` ref. I suggest using `--mRefCount <= 0`.

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/intrusive_...
compiled/intrusive_ptr.h:140: T* tmp = mPointer;
Nit: is there a specific reason why you're not using `std::swap` here?

[Wladimir Palant, 2016-02-02 17:55:22]

If that's all the issues you found after a quick look - I'm impressed, I must be
doing better than I thought :)

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:28: loop:
On 2016/02/02 11:11:22, René Jeschke wrote:
> Nit: this can be removed

Done.

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/StringMap....
compiled/StringMap.h:11: template<class Entry>
On 2016/02/02 11:11:22, René Jeschke wrote:
> Nit: The usage of `class` for declaring template parameters is discouraged,
> `typename` should be used instead. There are only very few special cases where
> `class` should/must be used.
> See: http://blogs.msdn.com/b/slippman/archive/2004/08/11/212768.aspx

Done.

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/intrusive_...
compiled/intrusive_ptr.h:24: if (--mRefCount == 0)
On 2016/02/02 11:11:23, René Jeschke wrote:
> Nit: having a check on `0` here feels wrong and may lead to unwanted behaviour
> if one tries to release a `0` ref. I suggest using `--mRefCount <= 0`.

It's quite the opposite I think - if we are already in the destructor (mRefCount
is 0) and somebody releases the ref again by mistake we won't delete a second
time. Either way, ref count dropping below 0 is always a bug - not sure whether
we can do anything sane in that case, other than crashing maybe.

https://codereview.adblockplus.org/29333474/diff/29335189/compiled/intrusive_...
compiled/intrusive_ptr.h:140: T* tmp = mPointer;
On 2016/02/02 11:11:24, René Jeschke wrote:
> Nit: is there a specific reason why you're not using `std::swap` here?

You have to ask Boost devs :)

I haven't heard about std::swap() before but I'll happily use it now.

[Wladimir Palant, 2016-02-04 15:08:11]

Patch Set 15 separated filter parsing and instantiation. Also, normalization
which had to be called explicitly via Filter.normalize() is part of the parsing
step now.  This made Filter.fromText() somewhat slower of course. Looking at our
codebase, there are quite a few places where we call Filter.fromText() but not
Filter.normalize(). In most cases the filter is definitely known however, so I
added Filter.getKnownFilter() as a shortcut for these scenarios - it's a simple
lookup, no parsing whatsoever. The only other important case is patterns.ini
parsing where we currently assume that the filter is already normalized - I hope
that parsing in C++ will by far outweight this minor slowdown here.

On the bright side, this change allowed me to disable C++ exceptions (without
STL containers there isn't much that could throw anyway). This reduced the code
size and improved performance almost to the old levels again.

[Wladimir Palant, 2016-02-04 19:24:41]

Looking at the generated code I realized that using the same class both for
strings owning their buffer and the ones that don't is a bad idea. Emscripten
was generating lots of unnecessary destructor calls. So in Patch Set 16 I've
spit it up again, there is DependentString and OwnedString now - only the latter
is a container and needs a destructor. Code size went down by 4 kB, no
significant impact on performance however.

[sergei, 2016-02-17 12:55:15]

I have not check the everything, however I would say a tremendous work is done
here!
I would propose to make a fork of 'adblockpluscore' with it as it is already. At
least it was very helpful to me to play a bit with it, I guess it would be also
helpful for others.

Most comments concern style and C++, though they may be have a small influence
on the generated JavaScript code.

https://codereview.adblockplus.org/29333474/diff/29336116/compile
File compile (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compile#newcode41
compile:41: '/bin/sh', '-c', os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh')])
emsdk_env.sh has no shebang and contains `pushd` which is not available in
Bourne shell, it would be better to use "bash" here, not "sh".

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:12: return std::move(OwnedString(buffer, len));
It's not necessary to use `std::move` for the return value.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:39: }, currChar);
Wouldn't it be better to call JS function on the rest on the string instead of a
per character?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:25: FilterNotifier.triggerListeners(getStringData($0),
filter, $3, $4);\
Why do we need to instantiate a new `filter` and cannot use `this`?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:35: void ParseDomains(const String& domains, char16_t
separator) const;
`String::value_type` instead of `char16_t` would be better, even more it's
`value_type` in cpp file.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:36: void AddSitekey(const String& sitekey) const;
I have removed const on the methods above, `mutable`, added constant operator[]
into StringMap, and slightly hacky constant getters `GetDomains` and
`GetSitekeys`, it seems working. It just looks confusing that such methods are
constant, their names say that they should not be constant.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/CSSPropert...
File compiled/CSSPropertyFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/CSSPropert...
compiled/CSSPropertyFilter.h:40: Type GetType() const;
Do you mind to add `override` (without `virtual`) everywhere for implementation
of virtual methods?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:58: : ref_counted(), mText(text)
`ref_counted()` is not necessary here.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:73: return std::move(result);
std::move is not necessary here.
BTW, in C++, here will be copy elision.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:78: NormalizeWhitespace(text);
For me personally, the approach here is really confusing in terms of C++ because
it can easily lead to something unexpected.

Eventually we copy `text` to `Filter::mText`, so I would propose to pass `text`
as a constant reference, copy it to `OwnedString` (`Filter::mText`) (here we can
trim it) and then operate on the `OwnedString` to remove undesired characters in
the middle and don't modify `DependentString`. BTW, instead of passing it as a
constant reference, it can be even r-value `OwnedString&&`.
I guess it won't cause a significant overhead.

Having that, I guess, we will be able to move non-constant methods from `String`
to `OwnedString` class and remove `DependentString(String& str,...)` and
`DependentString(value_type* buf, size_type len)` (constructors which accept
non-constant buffers).

What do you think about it?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:123: if
(reinterpret_cast<CSSPropertyFilter*>(filter.get())->IsGeneric())
it's better to use `static_cast` here.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/RegExpFilt...
File compiled/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/RegExpFilt...
compiled/RegExpFilter.cpp:54: int defaultTypeMask = INT_MAX & ~(TYPE_DOCUMENT |
TYPE_ELEMHIDE | TYPE_POPUP |
Should not it be const?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:45: 
What about adding protected destructor or virtual destructor? In that case
nobody will be able to accidentally forget to call the destructor of the derived
class.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:103: return pos;
Strictly speaking, if `pos > this->length()` then it should not find even empty
`str`, returning `npos` in that case seems reasonable, I guess.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:105: if (length() - pos < str.length())
We also should check whether `this->length()` is not zero.
If it's zero then `pos < length() - str.length();` (it's below in for-loop) is
likely to be true however the mBuf is likely to be nullptr.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:126: pos = length() - 1;
It seems safer to use something like `pos = min(pos, length() - 1);` because
`pos` can belong to [length(), npos).

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:153: DependentString(String& str, size_type pos = 0, size_type
len = npos)
I would add `explicit` for this particular method.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:191: bool is_invalid() const
what about moving it into base class String?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:198: return (mLen & FLAGS_MASK) == DELETED;
How can it actually happen? It seems it is never set to DELETED explicitly and
we don't subtract READ_ONLY from INVALID.

This method is not used actually.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:213: void resize(size_type newLength, bool copy)
I'm not sure about `copy` argument, it's called only once with `copy` equal to
`true`.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:224: std::memcpy(mBuf, oldBuffer, sizeof(value_type) *
std::min(oldLength, newLength));
If either destination or source buffer is nullptr then here is undefined
behaviour.

Here oldBuffer can be nullptr.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:240: std::memcpy(mBuf, str.mBuf, sizeof(value_type) *
length());
Here both mBuf and str.mBuf can be nullptr.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:297: if (!sourceLen)
it would be also good to check that source is not nullptr.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:11: struct _HashContainerIterator
What do you think about moving all internal structures into namespace instead of
using underscore "_" sign in the beginning of the names?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:27: {
It seems this destructor code is not needed.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:65: typedef _HashContainerIterator<Entry> iterator;
What do you think about renaming it to `const_iterator`?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:69: void operator=(const _HashContainer& other) {}
I guess, it's "noncopyable", it would  be better to remove the definition (the
body of the methods, {}) and leave only the declaration because otherwise, there
is still possibility to do it from one of _HashContainer class method.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:74: entry_type* mBuckets;
It can be std::unique_ptr<entry_type[]>, however I don't know how it will look
in JS.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:124: entry_type* oldBuckets = mBuckets;
here we can also use std::unique_ptr<[]>

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:127: mEntryCount = 0;
after executing of this method the member `mEntryCount` should be the same,
right? If so, it seems it would be better to remove changing of it here.
Although, it could be useful in some `assert` at the end of the method like
`assert(oldEntryCount == mEntryCount ...)`.

What do you think?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:138: *find_entry(entry.first) = entry;
It would be nice to explain it somewhere in comments, because it can be easily
missed if we decide to make some changes here.
I have not dived in the details, so I hope that `find_entry` uses `mBucketCount`
to prevent accessing elements beyond the allocated buffer. During resizing we
update the value of `mBucketCount`, so afterwards it can return another entry
for the same key, and to avoid it we need to rebuild the hashtable.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:149: *existing = entry;
Wouldn't it be better to firstly resize and only then assign a new value, the
probability to override the value seems lower in that case. Of course, we have
to call `find_entry` after resizing.

It would be also great to have somewhere tests to see the actual number of
collisions on a real data. I guess for us it's important to have the number of
inserted entries to be equal to mEntryCount. If there are collisions we probably
should do something, e.g. change the hash function or store collisions in one
entry.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:244: T& operator[](const String& key)
What do you think about having in addition a constant method with `assert` as in
`String.h` instead of inserting a new entry?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
compiled/intrusive_ptr.h:9: #pragma once
Yahoo! Finally we start it! I am looking forward to start to use `#pragma once`
in other C++ projects.
I also like to use some prefix, like "m" here for class fields.

Sorry, for offtopic.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
compiled/intrusive_ptr.h:89: intrusive_ptr(other).swap(*this);
JIC, `other` is just a reference here, I guess, to get the expected result it
should be written as
intrusive_ptr(std::move(other)).swap(*this);

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
compiled/intrusive_ptr.h:109: T* get() const
Although it's a common practice, I'm personally not sure that these methods
(pointer getters) should be `const`. It's very easy to make the following
mistake

class X {
 smart_ptr<M> m;
public:
 void constantMethod() const {
   m->modify();
 }
};

I think that usually the caller of `constantMethod` does not expect `X::m` to be
modified in `const` method, however, here it's possible.

What do you think about having two sets of methods `const T* get() const` and
`T* get()`?

In general it's up to you.

[Wladimir Palant, 2016-02-18 16:03:05]

With Patch Set 19 I rebased the changes so they should apply on top of
adblockpluscore repository now, and InvalidFilter.reason is an error code as
implemented in https://issues.adblockplus.org/ticket/3659. I also fixed the
class hierarchy - BlockingFilter class was missing. While addressing Sergei`s
comments I realized that Filter.fromText() will look up each filter twice, so I
made StringMap::find() return a "reference" that can be used to create the entry
without an additional lookup. This hack is only safe if certain conditions are
met but it again improved performance a bit.

[Wladimir Palant, 2016-02-18 16:07:17]

With Patch Set 19 I rebased the changes so they should apply on top of
adblockpluscore repository now. The new unit tests have been renamed from
filterClasses.js to filterClasses2.js - the former name is now occupied by the
old tests that should go away once I can confirm that the new ones cover
everything. Also, InvalidFilter.reason is an error code as implemented in
https://issues.adblockplus.org/ticket/3659.

I also fixed the class hierarchy - BlockingFilter class was missing. While
addressing Sergei`s comments I realized that Filter.fromText() will look up each
filter twice, so I made StringMap::find() return a "reference" that can be used
to create the entry without an additional lookup. This hack is only safe if
certain conditions are met but it again improved performance a bit.

https://codereview.adblockplus.org/29333474/diff/29336116/compile
File compile (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compile#newcode41
compile:41: '/bin/sh', '-c', os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh')])
On 2016/02/17 12:54:27, sergei wrote:
> emsdk_env.sh has no shebang and contains `pushd` which is not available in
> Bourne shell, it would be better to use "bash" here, not "sh".

Done. Not that this is production-ready code, for the "real" thing we would want
Windows support at the very least.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:12: return std::move(OwnedString(buffer, len));
On 2016/02/17 12:54:28, sergei wrote:
> It's not necessary to use `std::move` for the return value.

It got me confused that C++ generates different code depending on whether
std::move() is used. I checked this again. The C++ optimizer does a thorough job
so comparing the two variants is hard. However, it seems that you are right -
there are no additional memcpy operations no matter what, so the move semantics
are used. Also, it seems that the code without std::move() is slightly more
efficient (optimizer leaves a pointless assignment in the code otherwise).

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:39: }, currChar);
On 2016/02/17 12:54:27, sergei wrote:
> Wouldn't it be better to call JS function on the rest on the string instead of
a
> per character?

Not sure about that. "Rest of the string" isn't a JavaScript string so we cannot
simply call toLowerCase() on it. We would have the choice between having
JavaScript work with the typed array and convert each character separately (not
much different from this code but saves on call overhead) and converting the
buffer to a JavaScript string, calling toLowerCase() and converting back then
(significantly more complicated, performance improvement is questionable due to
overhead copying the string).

We almost never land in this branch, it's only six filters in all of EasyList.
Out of these filters two have three non-ASCII characters, the other ones only
one. So I think that keeping it simple here is the best solution.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:25: FilterNotifier.triggerListeners(getStringData($0),
filter, $3, $4);\
On 2016/02/17 12:54:31, sergei wrote:
> Why do we need to instantiate a new `filter` and cannot use `this`?

What is `this`? That's JavaScript code, all it got is a pointer to the C++
filter instance - there is no JavaScript wrapper for it, so we create one.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:35: void ParseDomains(const String& domains, char16_t
separator) const;
On 2016/02/17 12:54:29, sergei wrote:
> `String::value_type` instead of `char16_t` would be better, even more it's
> `value_type` in cpp file.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:36: void AddSitekey(const String& sitekey) const;
On 2016/02/17 12:54:30, sergei wrote:
> I have removed const on the methods above, `mutable`, added constant
operator[]
> into StringMap, and slightly hacky constant getters `GetDomains` and
> `GetSitekeys`, it seems working. It just looks confusing that such methods are
> constant, their names say that they should not be constant.

I'm not really sure how you've done that. If AddSitekey() isn't const then it
cannot be called from GetDomains(). Of course we can call it via GetDomains()
via const_cast but I'm not sure it's actually less of a hack. Did I miss some
other option?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/CSSPropert...
File compiled/CSSPropertyFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/CSSPropert...
compiled/CSSPropertyFilter.h:40: Type GetType() const;
On 2016/02/17 12:54:32, sergei wrote:
> Do you mind to add `override` (without `virtual`) everywhere for
implementation
> of virtual methods?

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:58: : ref_counted(), mText(text)
On 2016/02/17 12:54:37, sergei wrote:
> `ref_counted()` is not necessary here.

I preferred to spell this out explicitly nevertheless. Anyway, removed.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:73: return std::move(result);
On 2016/02/17 12:54:36, sergei wrote:
> std::move is not necessary here.
> BTW, in C++, here will be copy elision.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:78: NormalizeWhitespace(text);
On 2016/02/17 12:54:33, sergei wrote:
> For me personally, the approach here is really confusing in terms of C++
because
> it can easily lead to something unexpected.
> 
> Eventually we copy `text` to `Filter::mText`, so I would propose to pass
`text`
> as a constant reference, copy it to `OwnedString` (`Filter::mText`) (here we
can
> trim it) and then operate on the `OwnedString` to remove undesired characters
in
> the middle and don't modify `DependentString`. BTW, instead of passing it as a
> constant reference, it can be even r-value `OwnedString&&`.
> I guess it won't cause a significant overhead.
> 
> Having that, I guess, we will be able to move non-constant methods from
`String`
> to `OwnedString` class and remove `DependentString(String& str,...)` and
> `DependentString(value_type* buf, size_type len)` (constructors which accept
> non-constant buffers).
> 
> What do you think about it?

I've been through multiple iterations here, and the current version is the one
that saves significant overhead. While messing with the incoming buffers isn't
the best coding style, we can do so "for free" - they are allocated on the stack
and will be automatically released anyway as soon as we return. Once we allocate
our own buffer however we better make everything right - reallocating will be
expensive.

For example, how do we trim leading whitespace with an owned buffer? We have the
choice between moving the content and keeping two pointers around (one for the
buffer we need to release, another one for start of the content).
DependentString allows doing this very efficiently.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:123: if
(reinterpret_cast<CSSPropertyFilter*>(filter.get())->IsGeneric())
On 2016/02/17 12:54:34, sergei wrote:
> it's better to use `static_cast` here.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/RegExpFilt...
File compiled/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/RegExpFilt...
compiled/RegExpFilter.cpp:54: int defaultTypeMask = INT_MAX & ~(TYPE_DOCUMENT |
TYPE_ELEMHIDE | TYPE_POPUP |
On 2016/02/17 12:54:38, sergei wrote:
> Should not it be const?

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:45: 
On 2016/02/17 12:54:53, sergei wrote:
> What about adding protected destructor or virtual destructor? In that case
> nobody will be able to accidentally forget to call the destructor of the
derived
> class.

Nice one, I thought that having a protected constructor would already prevent
instantiating the String class - apparently not, I had this in a few places in
the codebase still.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:103: return pos;
On 2016/02/17 12:54:39, sergei wrote:
> Strictly speaking, if `pos > this->length()` then it should not find even
empty
> `str`, returning `npos` in that case seems reasonable, I guess.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:105: if (length() - pos < str.length())
On 2016/02/17 12:54:42, sergei wrote:
> We also should check whether `this->length()` is not zero.
> If it's zero then `pos < length() - str.length();` (it's below in for-loop) is
> likely to be true however the mBuf is likely to be nullptr.

If length() is 0 then length() - pos will be at most 0 and smaller than
str.length() which is non-zero. So this scenario is already covered.

Btw, I modified the conditions here to use addition rather than subtraction - we
don't want this to go into negative with unsigned numbers.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:126: pos = length() - 1;
On 2016/02/17 12:54:54, sergei wrote:
> It seems safer to use something like `pos = min(pos, length() - 1);` because
> `pos` can belong to [length(), npos).

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:153: DependentString(String& str, size_type pos = 0, size_type
len = npos)
On 2016/02/17 12:54:51, sergei wrote:
> I would add `explicit` for this particular method.

Why? I think that passing OwnedString to a function requiring DependentString
and having it converted implicitly is desired.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:191: bool is_invalid() const
On 2016/02/17 12:54:52, sergei wrote:
> what about moving it into base class String?

Given that it is only necessary for StringMap - why?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:198: return (mLen & FLAGS_MASK) == DELETED;
On 2016/02/17 12:54:49, sergei wrote:
> How can it actually happen? It seems it is never set to DELETED explicitly and
> we don't subtract READ_ONLY from INVALID.
> 
> This method is not used actually.

No, it's not - yet. The reason is that StringMap currently won't remove any
entries. Once we add this functionality we'll need a special value to mark
buckets with deleted entries.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:213: void resize(size_type newLength, bool copy)
On 2016/02/17 12:54:43, sergei wrote:
> I'm not sure about `copy` argument, it's called only once with `copy` equal to
> `true`.

Indeed, not any more - removed that parameter.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:224: std::memcpy(mBuf, oldBuffer, sizeof(value_type) *
std::min(oldLength, newLength));
On 2016/02/17 12:54:41, sergei wrote:
> If either destination or source buffer is nullptr then here is undefined
> behaviour.
> 
> Here oldBuffer can be nullptr.

oldLength is non-zero so oldBuffer cannot be nullptr. Destination buffer also
cannot be nullptr given that currently only growing the buffer is supported, not
shrinking it. I made this more clear by renaming the function into grow() and
removing the pointless std::min() here.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:240: std::memcpy(mBuf, str.mBuf, sizeof(value_type) *
length());
On 2016/02/17 12:54:40, sergei wrote:
> Here both mBuf and str.mBuf can be nullptr.

Should be fine as length() will be 0 in that case?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:297: if (!sourceLen)
On 2016/02/17 12:54:50, sergei wrote:
> it would be also good to check that source is not nullptr.

That would be a bug in the caller - meaning that we should crash, not ignore the
issue. I added an assert() call.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:11: struct _HashContainerIterator
On 2016/02/17 12:54:57, sergei wrote:
> What do you think about moving all internal structures into namespace instead
of
> using underscore "_" sign in the beginning of the names?

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:27: {
On 2016/02/17 12:54:55, sergei wrote:
> It seems this destructor code is not needed.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:65: typedef _HashContainerIterator<Entry> iterator;
On 2016/02/17 12:55:01, sergei wrote:
> What do you think about renaming it to `const_iterator`?

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:69: void operator=(const _HashContainer& other) {}
On 2016/02/17 12:54:59, sergei wrote:
> I guess, it's "noncopyable", it would  be better to remove the definition (the
> body of the methods, {}) and leave only the declaration because otherwise,
there
> is still possibility to do it from one of _HashContainer class method.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:74: entry_type* mBuckets;
On 2016/02/17 12:55:03, sergei wrote:
> It can be std::unique_ptr<entry_type[]>, however I don't know how it will look
> in JS.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:124: entry_type* oldBuckets = mBuckets;
On 2016/02/17 12:55:02, sergei wrote:
> here we can also use std::unique_ptr<[]>

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:127: mEntryCount = 0;
On 2016/02/17 12:54:58, sergei wrote:
> after executing of this method the member `mEntryCount` should be the same,
> right? If so, it seems it would be better to remove changing of it here.
> Although, it could be useful in some `assert` at the end of the method like
> `assert(oldEntryCount == mEntryCount ...)`.
> 
> What do you think?

It comes out identical right now, this is going to change once we start deleting
entries - resize() will "compress" the data by removing deleted entries then. So
I better leave this as is.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:138: *find_entry(entry.first) = entry;
On 2016/02/17 12:54:56, sergei wrote:
> It would be nice to explain it somewhere in comments, because it can be easily
> missed if we decide to make some changes here.
> I have not dived in the details, so I hope that `find_entry` uses
`mBucketCount`
> to prevent accessing elements beyond the allocated buffer. During resizing we
> update the value of `mBucketCount`, so afterwards it can return another entry
> for the same key, and to avoid it we need to rebuild the hashtable.

Not sure what these comments should explain but I renamed find_entry into
find_bucket - this will hopefully make it easier to understand.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:149: *existing = entry;
On 2016/02/17 12:55:00, sergei wrote:
> Wouldn't it be better to firstly resize and only then assign a new value, the
> probability to override the value seems lower in that case. Of course, we have
> to call `find_entry` after resizing.

I've restructured the code to assign after resizing - not sure whether it's
easier to follow the code now but at least the variable added is gone. As far as
performance goes, I don't really see what you expect to gain from it, one less
entry copy? That's negligible.

> It would be also great to have somewhere tests to see the actual number of
> collisions on a real data. I guess for us it's important to have the number of
> inserted entries to be equal to mEntryCount. If there are collisions we
probably
> should do something, e.g. change the hash function or store collisions in one
> entry.

Of course there are collisions, we are doing open hashing after all. With
EasyList data we do 238492 lookups, each lookup checks 2.4 buckets on average.
The median is only one check however, meaning that there are some very slow
lookups (maximum is 48 checks). 95% of the lookups result in 7 checks or less,
90% of the lookups in 5 checks or less.

48 checks is a lot of course, with our peak load factor of 0.76 the chances of
hitting a taken bucket 47 times in a row aren't exactly high. I suspect that
clustering might an issue so I already looked into changing the probing approach
- the difference wasn't really significant. Hash collisions on the other hand
don't seem to play a role here, otherwise we would see another lookup with 47
checks - but the next entry in the sorted list has 43 lookups.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:244: T& operator[](const String& key)
On 2016/02/17 12:55:04, sergei wrote:
> What do you think about having in addition a constant method with `assert` as
in
> `String.h` instead of inserting a new entry?

This is how STL does it, if you don't want to create a new entry you should be
using .find(). The problem is that the behavior will change in a non-obvious way
- e.g. you had some code operating on const StringMap and map[] was returning
nullptr for entries that cannot be found. Then you made a tiny change and your
StringMap is no longer const. And all the sudden the logic no longer works.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
compiled/intrusive_ptr.h:89: intrusive_ptr(other).swap(*this);
On 2016/02/17 12:55:07, sergei wrote:
> JIC, `other` is just a reference here, I guess, to get the expected result it
> should be written as
> intrusive_ptr(std::move(other)).swap(*this);

I'll just trust you on this one, currently I have no way of testing whether it
works correctly. I already had in mind however that this should be tested later.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/intrusive_...
compiled/intrusive_ptr.h:109: T* get() const
On 2016/02/17 12:55:06, sergei wrote:
> Although it's a common practice, I'm personally not sure that these methods
> (pointer getters) should be `const`. It's very easy to make the following
> mistake
> 
> class X {
>  smart_ptr<M> m;
> public:
>  void constantMethod() const {
>    m->modify();
>  }
> };
> 
> I think that usually the caller of `constantMethod` does not expect `X::m` to
be
> modified in `const` method, however, here it's possible.
> 
> What do you think about having two sets of methods `const T* get() const` and
> `T* get()`?
> 
> In general it's up to you.

Done, here and for the methods below. None of this code is really used at the
moment, we'll see how it works out once we actually start working actively with
these pointers.

[sergei, 2016-02-22 12:46:19]

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:12: return std::move(OwnedString(buffer, len));
On 2016/02/18 16:06:29, Wladimir Palant wrote:
> On 2016/02/17 12:54:28, sergei wrote:
> > It's not necessary to use `std::move` for the return value.
> 
> It got me confused that C++ generates different code depending on whether
> std::move() is used. I checked this again. The C++ optimizer does a thorough
job
> so comparing the two variants is hard. However, it seems that you are right -
> there are no additional memcpy operations no matter what, so the move
semantics
> are used. Also, it seems that the code without std::move() is slightly more
> efficient (optimizer leaves a pointless assignment in the code otherwise).

Yes, the behavior of C++ compiler regarding the return value depends on
optimization flags and on the actual code. Without any optimization copy-ctr is
used, however with optimization flags and simple code (e.g. only one return
statement in the function) it will likely choose copy elision
(http://en.cppreference.com/w/cpp/language/copy_elision), if it cannot do copy
elision and there is a move constructor then it will use move constructor.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:39: }, currChar);
On 2016/02/18 16:06:30, Wladimir Palant wrote:
> On 2016/02/17 12:54:27, sergei wrote:
> > Wouldn't it be better to call JS function on the rest on the string instead
of
> a
> > per character?
> 
> Not sure about that. "Rest of the string" isn't a JavaScript string so we
cannot
> simply call toLowerCase() on it. We would have the choice between having
> JavaScript work with the typed array and convert each character separately
(not
> much different from this code but saves on call overhead) and converting the
> buffer to a JavaScript string, calling toLowerCase() and converting back then
> (significantly more complicated, performance improvement is questionable due
to
> overhead copying the string).
I thought rather about surrogate pairs, e.g.
http://www.fileformat.info/info/unicode/char/118A0/index.htm, though it might be
not our case.
> 
> We almost never land in this branch, it's only six filters in all of EasyList.
> Out of these filters two have three non-ASCII characters, the other ones only
> one. So I think that keeping it simple here is the best solution.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:25: FilterNotifier.triggerListeners(getStringData($0),
filter, $3, $4);\
On 2016/02/18 16:06:38, Wladimir Palant wrote:
> On 2016/02/17 12:54:31, sergei wrote:
> > Why do we need to instantiate a new `filter` and cannot use `this`?
> 
> What is `this`? That's JavaScript code, all it got is a pointer to the C++
> filter instance - there is no JavaScript wrapper for it, so we create one.

Acknowledged, sorry, overlooked it.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:36: void AddSitekey(const String& sitekey) const;
On 2016/02/18 16:06:32, Wladimir Palant wrote:
> On 2016/02/17 12:54:30, sergei wrote:
> > I have removed const on the methods above, `mutable`, added constant
> operator[]
> > into StringMap, and slightly hacky constant getters `GetDomains` and
> > `GetSitekeys`, it seems working. It just looks confusing that such methods
are
> > constant, their names say that they should not be constant.
> 
> I'm not really sure how you've done that. If AddSitekey() isn't const then it
> cannot be called from GetDomains(). Of course we can call it via GetDomains()
> via const_cast but I'm not sure it's actually less of a hack. Did I miss some
> other option?

Yeah, I did it using const_cast, it's difficult to say what is better. Let's
leave it as is.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:78: NormalizeWhitespace(text);
On 2016/02/18 16:06:43, Wladimir Palant wrote:
> On 2016/02/17 12:54:33, sergei wrote:
> > For me personally, the approach here is really confusing in terms of C++
> because
> > it can easily lead to something unexpected.
> > 
> > Eventually we copy `text` to `Filter::mText`, so I would propose to pass
> `text`
> > as a constant reference, copy it to `OwnedString` (`Filter::mText`) (here we
> can
> > trim it) and then operate on the `OwnedString` to remove undesired
characters
> in
> > the middle and don't modify `DependentString`. BTW, instead of passing it as
a
> > constant reference, it can be even r-value `OwnedString&&`.
> > I guess it won't cause a significant overhead.
> > 
> > Having that, I guess, we will be able to move non-constant methods from
> `String`
> > to `OwnedString` class and remove `DependentString(String& str,...)` and
> > `DependentString(value_type* buf, size_type len)` (constructors which accept
> > non-constant buffers).
> > 
> > What do you think about it?
> 
> I've been through multiple iterations here, and the current version is the one
> that saves significant overhead. While messing with the incoming buffers isn't
> the best coding style, we can do so "for free" - they are allocated on the
stack
> and will be automatically released anyway as soon as we return. Once we
allocate
> our own buffer however we better make everything right - reallocating will be
> expensive.

Clear.

BTW, can we avoid copying of strings here at all? In the extension we read
filters from a file, and we can read the file into a contiguous buffer, then we
can create `DependentString`s which use that buffer and even modify it in place
(it's our buffer on the heap). It looks fragile but it should be very efficient.
Of course we should do something to have an ability to add and remove filters.

> For example, how do we trim leading whitespace with an owned buffer? We have
the
> choice between moving the content and keeping two pointers around (one for the
> buffer we need to release, another one for start of the content).
> DependentString allows doing this very efficiently.
We can do it similar way as we do it now. Currently we find `start` and `end` of
the expected trimmed and normalized string in `NormalizeWhitespace`, then call
`DependentString::reset(text, start, end - start);` and then copy the piece of
buffer determined by fields of `DependentString` into `Filter::mText`. I
proposed to find offset (`start`) and the length (`start - end`) of the expected
trimmed string and then copy it into `Filter::mText` without modifying of
`DependentString` and its buffer, and then remove undesired characters in the
middle of the string. The latter action means the possibility for overhead in
the unused tail of `OwnedString`, how often does it actually happen?. And to do
that we need to add `OwnedString::OwnedString(const String& src, size_type
offset, size_type length)` or use `OwnedString(DependentString(text, offset,
length))`.

Anyway, since we know what is happening on the caller side, I would propose to
leave it as is because it's more descriptive now.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:105: if (length() - pos < str.length())
On 2016/02/18 16:06:47, Wladimir Palant wrote:
> On 2016/02/17 12:54:42, sergei wrote:
> > We also should check whether `this->length()` is not zero.
> > If it's zero then `pos < length() - str.length();` (it's below in for-loop)
is
> > likely to be true however the mBuf is likely to be nullptr.
> 
> If length() is 0 then length() - pos will be at most 0 and smaller than
> str.length() which is non-zero. So this scenario is already covered.
> 
> Btw, I modified the conditions here to use addition rather than subtraction -
we
> don't want this to go into negative with unsigned numbers.

I wanted to have at the beginning of the method the following code

bool isFindable = pos <= length() && str.length() <= length() - pos;
if (!isFindable)
  return npos;

Any integer overflow with unsigned integers in C++ results in unsigned integer
(and it's undefined behavior for signed integers). So, `pos + str.length() >
length()` can be false but it still does not protect from accessing beyond the
size of mBuf, e.g.
length = 10;
pos = npos - 5 (pos > length)
str.length = 10;

pos + str.length = 4 ( 4 <= length)


In addition I have just realized that in our particular case we cannot get
integer overflow because the length is limited by LENGTH_MASK, though we should
verify that `pos <= LENGTH_MASK` (actually, the proposed `isFindable` covers
it).

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:153: DependentString(String& str, size_type pos = 0, size_type
len = npos)
On 2016/02/18 16:06:51, Wladimir Palant wrote:
> On 2016/02/17 12:54:51, sergei wrote:
> > I would add `explicit` for this particular method.
> 
> Why? I think that passing OwnedString to a function requiring DependentString
> and having it converted implicitly is desired.

May be it's desired. It seems it's not actually used now.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:191: bool is_invalid() const
On 2016/02/18 16:06:52, Wladimir Palant wrote:
> On 2016/02/17 12:54:52, sergei wrote:
> > what about moving it into base class String?
> 
> Given that it is only necessary for StringMap - why?

Because flag values are defined in `String`, flag value is stored in the member
of `String` and there is `String::is_readOnly` method, it seems `String` class
is a better place for methods which work with flags.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:198: return (mLen & FLAGS_MASK) == DELETED;
On 2016/02/18 16:06:55, Wladimir Palant wrote:
> On 2016/02/17 12:54:49, sergei wrote:
> > How can it actually happen? It seems it is never set to DELETED explicitly
and
> > we don't subtract READ_ONLY from INVALID.
> > 
> > This method is not used actually.
> 
> No, it's not - yet. The reason is that StringMap currently won't remove any
> entries. Once we add this functionality we'll need a special value to mark
> buckets with deleted entries.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:224: std::memcpy(mBuf, oldBuffer, sizeof(value_type) *
std::min(oldLength, newLength));
On 2016/02/18 16:06:57, Wladimir Palant wrote:
> On 2016/02/17 12:54:41, sergei wrote:
> > If either destination or source buffer is nullptr then here is undefined
> > behaviour.
> > 
> > Here oldBuffer can be nullptr.
> 
> oldLength is non-zero so oldBuffer cannot be nullptr. Destination buffer also
> cannot be nullptr given that currently only growing the buffer is supported,
not
> shrinking it. I made this more clear by renaming the function into grow() and
> removing the pointless std::min() here.

Acknowledged, sorry, overlooked.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:240: std::memcpy(mBuf, str.mBuf, sizeof(value_type) *
length());
On 2016/02/18 16:06:54, Wladimir Palant wrote:
> On 2016/02/17 12:54:40, sergei wrote:
> > Here both mBuf and str.mBuf can be nullptr.
> 
> Should be fine as length() will be 0 in that case?

According to http://en.cppreference.com/w/cpp/string/byte/memcpy - no,
"If either dest or src is a null pointer, the behavior is undefined, even if
count is zero."

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:297: if (!sourceLen)
On 2016/02/18 16:06:50, Wladimir Palant wrote:
> On 2016/02/17 12:54:50, sergei wrote:
> > it would be also good to check that source is not nullptr.
> 
> That would be a bug in the caller - meaning that we should crash, not ignore
the
> issue. I added an assert() call.

Good.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:127: mEntryCount = 0;
On 2016/02/18 16:07:08, Wladimir Palant wrote:
> On 2016/02/17 12:54:58, sergei wrote:
> > after executing of this method the member `mEntryCount` should be the same,
> > right? If so, it seems it would be better to remove changing of it here.
> > Although, it could be useful in some `assert` at the end of the method like
> > `assert(oldEntryCount == mEntryCount ...)`.
> > 
> > What do you think?
> 
> It comes out identical right now, this is going to change once we start
deleting
> entries - resize() will "compress" the data by removing deleted entries then.
So
> I better leave this as is.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:149: *existing = entry;
On 2016/02/18 16:07:06, Wladimir Palant wrote:
> On 2016/02/17 12:55:00, sergei wrote:
> > Wouldn't it be better to firstly resize and only then assign a new value,
the
> > probability to override the value seems lower in that case. Of course, we
have
> > to call `find_entry` after resizing.
> 
> I've restructured the code to assign after resizing - not sure whether it's
> easier to follow the code now but at least the variable added is gone. As far
as
> performance goes, I don't really see what you expect to gain from it, one less
> entry copy? That's negligible.
> 
> > It would be also great to have somewhere tests to see the actual number of
> > collisions on a real data. I guess for us it's important to have the number
of
> > inserted entries to be equal to mEntryCount. If there are collisions we
> probably
> > should do something, e.g. change the hash function or store collisions in
one
> > entry.
> 
> Of course there are collisions, we are doing open hashing after all. With
> EasyList data we do 238492 lookups, each lookup checks 2.4 buckets on average.
> The median is only one check however, meaning that there are some very slow
> lookups (maximum is 48 checks). 95% of the lookups result in 7 checks or less,
> 90% of the lookups in 5 checks or less.
> 
> 48 checks is a lot of course, with our peak load factor of 0.76 the chances of
> hitting a taken bucket 47 times in a row aren't exactly high. I suspect that
> clustering might an issue so I already looked into changing the probing
approach
> - the difference wasn't really significant. Hash collisions on the other hand
> don't seem to play a role here, otherwise we would see another lookup with 47
> checks - but the next entry in the sorted list has 43 lookups.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/StringMap....
compiled/StringMap.h:244: T& operator[](const String& key)
On 2016/02/18 16:07:11, Wladimir Palant wrote:
> On 2016/02/17 12:55:04, sergei wrote:
> > What do you think about having in addition a constant method with `assert`
as
> in
> > `String.h` instead of inserting a new entry?
> 
> This is how STL does it, if you don't want to create a new entry you should be
> using .find(). The problem is that the behavior will change in a non-obvious
way
> - e.g. you had some code operating on const StringMap and map[] was returning
> nullptr for entries that cannot be found. Then you made a tiny change and your
> StringMap is no longer const. And all the sudden the logic no longer works.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:23: String::size_type end)
Wouldn't it be better to replace these arguments by one `DependentString`?

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:38: }
What about assert(mLen > 0 || mBuf == nullptr)?

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:161: READ_WRITE
BTW, `READ_WRITE` here should be

str.is_readOnly() ? READ_ONLY : READ_WRITE

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:275: }
I think here is a memory leak.
`OwnedString(str.length())` allocates the buffer and then we override the
pointer on it by `mBuf = str.mBuf;`.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:277: OwnedString(const char* source, size_type len)
I'm not sure whether we need this method because it does not support UTF-8 and
it's used only once in `to_string(unsigned int)` (in ActiveFilter.cpp).

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap....
compiled/StringMap.h:245: void assign(const String& key, const T& value)
I have noticed a couple of asserts here in the repository, what about removing
`key` argument and use only `value`?

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap....
compiled/StringMap.h:247: mMap->assign(const_cast<entry_type*>(super::mEntry),
entry_type(key, value));
It's not necessary to use `const_cast` here. We can remove `const` modifier for
`HashContainerReference::mEntry` and use `typedef HashContainerReference<const
Entry> const_reference;` inside `HashContainer`.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap....
compiled/StringMap.h:247: mMap->assign(const_cast<entry_type*>(super::mEntry),
entry_type(key, value));
`super::mEntry` is also accessible by `this->mEntry`.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h
File compiled/bindings.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:18: typedef void* TYPEID;
I think most part of this file should be in some namespace because it's very
auxiliary.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:95: void* function)
Storing a function pointer in `void*` looks incorrect, may be it works so far
but it definitely does not work in general.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:100: if (!*reinterpret_cast<int*>(function))
What does this check test and why do we need it?

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:260: std::map<TYPEID,ClassInfo> classes;
It's not a good idea to have a global variable in a header.

https://codereview.adblockplus.org/29333474/diff/29336598/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29333474/diff/29336598/test_wrapper.js#new...
test_wrapper.js:19: return origResolve.apply(this, params);
Actually to run tests I have to put there some conditions from removed test.js.
The following variant seems not perfect, merely added to have it running.

   let params = Array.slice(arguments);
   if (params[0] == "filterClasses")
-    params[0] = "filterClassesNew";
+    params[0] = "../lib/filterClassesNew";
   else if (params[0] == "../lib/filterClasses")
     params[0] = "../lib/filterClassesNew";
+  else if (params[0] == "compiled")
+    params[0] = "../lib/compiled";
+  else if (params[0] == "filterNotifier")
+    params[0] = "../lib/filterNotifier";
   return origResolve.apply(this, params);

[Wladimir Palant, 2016-02-23 12:37:51]

Uploaded Patch Set 21 : Addressed Sergei's comments again and added some asserts

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:39: }, currChar);
On 2016/02/22 12:45:32, sergei wrote:
> I thought rather about surrogate pairs, e.g.
> http://www.fileformat.info/info/unicode/char/118A0/index.htm, though it might
be
> not our case.

Interesting. I was under the impression that JavaScript won't deal with Unicode
characters beyond 0x10000 but apparently it does, merely in a somewhat
unexpected way. "\u{118A0}" will produce a single-character string which is
represented at two characters internally, and that's how we get it in C++.

However, toLowerCase() doesn't have an effect on this character. I also tried
"\u{10400}" which definitely should have been mapped to "\u{10428}" (Mozilla has
a mapping for it in
http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsUnicode...)
- nothing here either, neither in Firefox nor in Chrome. I found the issue under
http://hg.mozilla.org/mozilla-central/file/789a12291942/js/src/jsstr.cpp#l637 -
instead of calling unicode::ToLowerCase() on the entire buffer (which would
consider surrogate pairs correctly) it converts each character separately.
Consequently, surrogate pairs aren't being handled (intentionally, I guess). So
at the moment we just shouldn't care I think.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:78: NormalizeWhitespace(text);
On 2016/02/22 12:45:36, sergei wrote:
> BTW, can we avoid copying of strings here at all? In the extension we read
> filters from a file, and we can read the file into a contiguous buffer, then
we
> can create `DependentString`s which use that buffer and even modify it in
place
> (it's our buffer on the heap). It looks fragile but it should be very
efficient.

Reading that file will eventually move into C++ so at that point we could try
tricks like that. There is an encoding issue however: the file uses UTF-8 which
we will have to decode anyway. And even without it, it would merely lead to
memory fragmentation - that file contains way more info that the filter strings,
and we wouldn't be able to reuse that memory easily. I think a fixed buffer
(e.g. 128 kB) which we would read chunks of the file into, parse and then reuse
will be way more memory-efficient.

> I
> proposed to find offset (`start`) and the length (`start - end`) of the
expected
> trimmed string and then copy it into `Filter::mText` without modifying of
> `DependentString` and its buffer,

I had this implemented like that, to some degree. Main problem is: we have to
perform the normalization before creating the filter, in order to do the lookup
in knownFilters. We only create the filter if that lookup yields no result. I'd
rather not create an owned buffer only to free it again because we didn't need
it.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:105: if (length() - pos < str.length())
On 2016/02/22 12:45:46, sergei wrote:
> In addition I have just realized that in our particular case we cannot get
> integer overflow because the length is limited by LENGTH_MASK, though we
should
> verify that `pos <= LENGTH_MASK` (actually, the proposed `isFindable` covers
> it).

Yes, in our case an integer overflow is luckily impossible because we aren't
using the upper two bits of the length. That's exactly why I changed the
comparisons to use addition rather than subtraction, this way it is safe.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:153: DependentString(String& str, size_type pos = 0, size_type
len = npos)
On 2016/02/22 12:45:45, sergei wrote:
> May be it's desired. It seems it's not actually used now.

It's being used in various getters, e.g. Filter::GetText (the const variant
below actually).

Is your concern that we would end up unintentionally creating a persistent
DependentString instance based on a temporary OwnedString instance?

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:191: bool is_invalid() const
On 2016/02/22 12:45:49, sergei wrote:
> On 2016/02/18 16:06:52, Wladimir Palant wrote:
> > On 2016/02/17 12:54:52, sergei wrote:
> > > what about moving it into base class String?
> > 
> > Given that it is only necessary for StringMap - why?
> 
> Because flag values are defined in `String`, flag value is stored in the
member
> of `String` and there is `String::is_readOnly` method, it seems `String` class
> is a better place for methods which work with flags.

Done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:240: std::memcpy(mBuf, str.mBuf, sizeof(value_type) *
length());
On 2016/02/22 12:45:48, sergei wrote:
> On 2016/02/18 16:06:54, Wladimir Palant wrote:
> > On 2016/02/17 12:54:40, sergei wrote:
> > > Here both mBuf and str.mBuf can be nullptr.
> > 
> > Should be fine as length() will be 0 in that case?
> 
> According to http://en.cppreference.com/w/cpp/string/byte/memcpy - no,
> "If either dest or src is a null pointer, the behavior is undefined, even if
> count is zero."

Done.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:23: String::size_type end)
On 2016/02/22 12:45:56, sergei wrote:
> Wouldn't it be better to replace these arguments by one `DependentString`?

Done.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:38: }
On 2016/02/22 12:46:01, sergei wrote:
> What about assert(mLen > 0 || mBuf == nullptr)?

Somewhat problematic since asserts take string literals and those haven't been
defined at this point :)

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:275: }
On 2016/02/22 12:45:59, sergei wrote:
> I think here is a memory leak.
> `OwnedString(str.length())` allocates the buffer and then we override the
> pointer on it by `mBuf = str.mBuf;`.

Done.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/String.h#n...
compiled/String.h:277: OwnedString(const char* source, size_type len)
On 2016/02/22 12:46:00, sergei wrote:
> I'm not sure whether we need this method because it does not support UTF-8 and
> it's used only once in `to_string(unsigned int)` (in ActiveFilter.cpp).

Yes, it's somewhat of a hack - removed.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap....
compiled/StringMap.h:245: void assign(const String& key, const T& value)
On 2016/02/22 12:46:03, sergei wrote:
> I have noticed a couple of asserts here in the repository, what about removing
> `key` argument and use only `value`?

As explained under https://codereview.adblockplus.org/29333474/#msg21, this
approach is quite a hack. However, I don't see a better way - the key passed
into find() and the one here aren't the same. The former refers to a temporary
buffer which will go away, the latter to an actual OwnedString that we can store
a reference to (it's only created if the lookup via find() doesn't yield a
result). The asserts I added verify that both actually have the same value which
makes this somewhat safer.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap....
compiled/StringMap.h:247: mMap->assign(const_cast<entry_type*>(super::mEntry),
entry_type(key, value));
On 2016/02/22 12:46:02, sergei wrote:
> `super::mEntry` is also accessible by `this->mEntry`.

Done.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/StringMap....
compiled/StringMap.h:247: mMap->assign(const_cast<entry_type*>(super::mEntry),
entry_type(key, value));
On 2016/02/22 12:46:05, sergei wrote:
> It's not necessary to use `const_cast` here. We can remove `const` modifier
for
> `HashContainerReference::mEntry` and use `typedef HashContainerReference<const
> Entry> const_reference;` inside `HashContainer`.

Done.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h
File compiled/bindings.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:18: typedef void* TYPEID;
On 2016/02/22 12:46:12, sergei wrote:
> I think most part of this file should be in some namespace because it's very
> auxiliary.

Done.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:95: void* function)
On 2016/02/22 12:46:14, sergei wrote:
> Storing a function pointer in `void*` looks incorrect, may be it works so far
> but it definitely does not work in general.

It isn't actually a function pointer, rather a pointer to a function pointer.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:100: if (!*reinterpret_cast<int*>(function))
On 2016/02/22 12:46:10, sergei wrote:
> What does this check test and why do we need it?

I've added a comment explaining this.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:260: std::map<TYPEID,ClassInfo> classes;
On 2016/02/22 12:46:11, sergei wrote:
> It's not a good idea to have a global variable in a header.

Why not? This header is meant to be included from bindings.cpp only.

Originally I meant to make this an instance variable of BindingsInitializer -
until I realized that the classes we instantiate in its constructor won't have
access to that variable. It could be a static variable instead but this has the
same issues.

https://codereview.adblockplus.org/29333474/diff/29336598/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29333474/diff/29336598/test_wrapper.js#new...
test_wrapper.js:19: return origResolve.apply(this, params);
On 2016/02/22 12:46:15, sergei wrote:
> Actually to run tests I have to put there some conditions from removed
test.js.

Are you running this file directly? You shouldn't be doing that, please run "npm
test" instead.

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/RegExpFilt...
File compiled/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/RegExpFilt...
compiled/RegExpFilter.cpp:128: ActiveFilter::ToLower(DependentString(text,
data.mDomainsStart,
This should be valueStart rather than data.mDomainsStart of course, I already
fixed it in the repository.

[sergei, 2016-02-23 15:07:40]

Only a couple of comments.

The next part I plan to deeply review is bindings.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:39: }, currChar);
On 2016/02/23 12:37:20, Wladimir Palant wrote:
> On 2016/02/22 12:45:32, sergei wrote:
> > I thought rather about surrogate pairs, e.g.
> > http://www.fileformat.info/info/unicode/char/118A0/index.htm, though it
might
> be
> > not our case.
> 
> Interesting. I was under the impression that JavaScript won't deal with
Unicode
> characters beyond 0x10000 but apparently it does, merely in a somewhat
> unexpected way. "\u{118A0}" will produce a single-character string which is
> represented at two characters internally, and that's how we get it in C++.
> 
> However, toLowerCase() doesn't have an effect on this character. I also tried
> "\u{10400}" which definitely should have been mapped to "\u{10428}" (Mozilla
has
> a mapping for it in
>
http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsUnicode...)
> - nothing here either, neither in Firefox nor in Chrome. I found the issue
under
> http://hg.mozilla.org/mozilla-central/file/789a12291942/js/src/jsstr.cpp#l637
-
> instead of calling unicode::ToLowerCase() on the entire buffer (which would
> consider surrogate pairs correctly) it converts each character separately.
> Consequently, surrogate pairs aren't being handled (intentionally, I guess).
So
> at the moment we just shouldn't care I think.

Agree.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/Filter.cpp...
compiled/Filter.cpp:78: NormalizeWhitespace(text);
On 2016/02/23 12:37:20, Wladimir Palant wrote:
> On 2016/02/22 12:45:36, sergei wrote:
> > BTW, can we avoid copying of strings here at all? In the extension we read
> > filters from a file, and we can read the file into a contiguous buffer, then
> we
> > can create `DependentString`s which use that buffer and even modify it in
> place
> > (it's our buffer on the heap). It looks fragile but it should be very
> efficient.
> 
> Reading that file will eventually move into C++ so at that point we could try
> tricks like that. There is an encoding issue however: the file uses UTF-8
which
> we will have to decode anyway. And even without it, it would merely lead to
> memory fragmentation - that file contains way more info that the filter
strings,
> and we wouldn't be able to reuse that memory easily. I think a fixed buffer
> (e.g. 128 kB) which we would read chunks of the file into, parse and then
reuse
> will be way more memory-efficient.
> 
> > I
> > proposed to find offset (`start`) and the length (`start - end`) of the
> expected
> > trimmed string and then copy it into `Filter::mText` without modifying of
> > `DependentString` and its buffer,
> 
> I had this implemented like that, to some degree. Main problem is: we have to
> perform the normalization before creating the filter, in order to do the
lookup
> in knownFilters. We only create the filter if that lookup yields no result.
I'd
> rather not create an owned buffer only to free it again because we didn't need
> it.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:105: if (length() - pos < str.length())
On 2016/02/23 12:37:21, Wladimir Palant wrote:
> On 2016/02/22 12:45:46, sergei wrote:
> > In addition I have just realized that in our particular case we cannot get
> > integer overflow because the length is limited by LENGTH_MASK, though we
> should
> > verify that `pos <= LENGTH_MASK` (actually, the proposed `isFindable` covers
> > it).
> 
> Yes, in our case an integer overflow is luckily impossible because we aren't
> using the upper two bits of the length. That's exactly why I changed the
> comparisons to use addition rather than subtraction, this way it is safe.

As a matter of form we still need to check that `pos <= LENGTH_MASK`.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:153: DependentString(String& str, size_type pos = 0, size_type
len = npos)
On 2016/02/23 12:37:26, Wladimir Palant wrote:
> On 2016/02/22 12:45:45, sergei wrote:
> > May be it's desired. It seems it's not actually used now.
> 
> It's being used in various getters, e.g. Filter::GetText (the const variant
> below actually).
Yes, the variant with const, but this constructor is not used.
>
> Is your concern that we would end up unintentionally creating a persistent
> DependentString instance based on a temporary OwnedString instance?
No, it's difficult to say what was confusing here. Let's keep it, actually it
looks good and useful.
I guess, I wanted to see where it's used because it can help to identify whether
we really need `DependentString` or the argument can be simply `String&`.

BTW, just wonder, why do we use `EMSCRIPTEN_KEEPALIVE const DependentString`
instead of `EMSCRIPTEN_KEEPALIVE const String&`? I have tried to change it but
could not get all tests passing because it requires to have a good understanding
of what is happening in bindings.{h,js} where I have not carefully looked yet.

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h
File compiled/bindings.h (right):

https://codereview.adblockplus.org/29333474/diff/29336598/compiled/bindings.h...
compiled/bindings.h:260: std::map<TYPEID,ClassInfo> classes;
On 2016/02/23 12:37:42, Wladimir Palant wrote:
> On 2016/02/22 12:46:11, sergei wrote:
> > It's not a good idea to have a global variable in a header.
> 
> Why not? This header is meant to be included from bindings.cpp only.
It should be somewhere emphasized. May be rename the file to something like
"bindings.ipp", although "ipp" is usually used for headers with inline
implementations.

> 
> Originally I meant to make this an instance variable of BindingsInitializer -
> until I realized that the classes we instantiate in its constructor won't have
> access to that variable. It could be a static variable instead but this has
the
> same issues.

https://codereview.adblockplus.org/29333474/diff/29336598/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29333474/diff/29336598/test_wrapper.js#new...
test_wrapper.js:19: return origResolve.apply(this, params);
On 2016/02/23 12:37:45, Wladimir Palant wrote:
> On 2016/02/22 12:46:15, sergei wrote:
> > Actually to run tests I have to put there some conditions from removed
> test.js.
> 
> Are you running this file directly? You shouldn't be doing that, please run
"npm
> test" instead.

Yes, thanks, overlooked it in package.json.

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/String.h#n...
compiled/String.h:277: str.mLen = READ_WRITE | 0;
I would like to use `std::swap` here, but the current variant is also fine for
me.

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/StringMap....
compiled/StringMap.h:266: assert(mHash == mMap->hash(key),
It might be better to compare keys instead of hashes.

[sergei, 2016-02-23 15:45:59]

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/debug.h
File compiled/debug.h (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/debug.h#ne...
compiled/debug.h:43: }
BTW, I would #define `assert` to nothing when DEBUG is not defined. In this case
its arguments definitely won't be evaluated, otherwise it can have a huge
performance influence.

[Wladimir Palant, 2016-02-23 21:35:06]

I uploaded renaming bindings.h into bindings.ipp as a separate patchset so you
can see changes to bindings.h on Patch Set 22. In Patch Set 23 this file is
unchanged, merely renamed.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:105: if (length() - pos < str.length())
On 2016/02/23 15:07:27, sergei wrote:
> As a matter of form we still need to check that `pos <= LENGTH_MASK`.

True, done.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/String.h#n...
compiled/String.h:153: DependentString(String& str, size_type pos = 0, size_type
len = npos)
On 2016/02/23 15:07:26, sergei wrote:
> BTW, just wonder, why do we use `EMSCRIPTEN_KEEPALIVE const DependentString`
> instead of `EMSCRIPTEN_KEEPALIVE const String&`? I have tried to change it but
> could not get all tests passing because it requires to have a good
understanding
> of what is happening in bindings.{h,js} where I have not carefully looked yet.

bindings.h currently enforces the return value to be either DependentString or
OwnedString, other string types won't be accepted. Reason is that it needs to
know the calling convention and it needs to know whether the string destructor
should be called. Returning String& is a good idea, it changes the calling
convention however - there is a return value in this case. So bindings.h has to
learn that scenario.

I've done it now, there was unexpected trouble however. With a reference as
result Filter::GetText() will no longer error out if called with invalid
parameters, consequently my hack getting the function name doesn't work. I
realized that the same thing can be done more reliably by filling up the stack
before calling the function.

I also marked these two constructors explicit as nothing is using them
implicitly any more.

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/String.h#n...
compiled/String.h:277: str.mLen = READ_WRITE | 0;
On 2016/02/23 15:07:34, sergei wrote:
> I would like to use `std::swap` here, but the current variant is also fine for
> me.

I don't see how that could be used in a meaningful way here.

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/StringMap....
compiled/StringMap.h:266: assert(mHash == mMap->hash(key),
On 2016/02/23 15:07:36, sergei wrote:
> It might be better to compare keys instead of hashes.

a) We cannot really keep a reference to the key since it might be referring to a
temporary buffer which is subject to change - this code is meant to protect
against this very scenario.

b) If it is a different key with the same hash - we don't really care. The
important thing is that its hash matches the bucket it is in so that it can be
found there.

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/debug.h
File compiled/debug.h (right):

https://codereview.adblockplus.org/29333474/diff/29337508/compiled/debug.h#ne...
compiled/debug.h:43: }
On 2016/02/23 15:45:57, sergei wrote:
> BTW, I would #define `assert` to nothing when DEBUG is not defined. In this
case
> its arguments definitely won't be evaluated, otherwise it can have a huge
> performance influence.

Done.

[sergei, 2016-06-16 21:17:35]

Next portion of comments.

Please pay attention to the convention regarding working with ref_counted. -
Function argument should have positive reference counter to avoid misdeletion.
- The return value should also have positive reference counter and the caller is
responsible to call ReleaseRef on the received return value.
- If the object should be stored somewhere, e.g. as a member one should call
AddRef before storing and ReleaseRef when it's not needed anymore.

What about "rebase" of it? I guess, filter parsing code can have important
changes after it, so it would be better to discuss new code.

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
compiled/ActiveFilter.h:36: void AddSitekey(const String& sitekey) const;
On 2016/02/22 12:45:35, sergei wrote:
> On 2016/02/18 16:06:32, Wladimir Palant wrote:
> > On 2016/02/17 12:54:30, sergei wrote:
> > > I have removed const on the methods above, `mutable`, added constant
> > operator[]
> > > into StringMap, and slightly hacky constant getters `GetDomains` and
> > > `GetSitekeys`, it seems working. It just looks confusing that such methods
> are
> > > constant, their names say that they should not be constant.
> > 
> > I'm not really sure how you've done that. If AddSitekey() isn't const then
it
> > cannot be called from GetDomains(). Of course we can call it via
GetDomains()
> > via const_cast but I'm not sure it's actually less of a hack. Did I miss
some
> > other option?
> 
> Yeah, I did it using const_cast, it's difficult to say what is better. Let's
> leave it as is.

Actually there is another option with only one mutable member and better looking
const-correctness but I'm not sure it is worth it now. I would say, let's keep
it in mind for present.

The option is to have some auxiliary ActiveFilter::Data structure and derived
from it RegExpFilter::Data structure (RegExpFilterData is a good canditate). It
should be possible to inject ActiveFilter::Data* using protected constructor.
Only the pointer to m_data should be mutable. In this case we can call such
non-const methods of Data from const methods of ActiveFilter and any Data will
be const-correct without mutable. It also removes inheritance from
RegExpFilterData and can have a positive effect if we make
ActiveFilter::GetDomains and ActiveFilter::GetSitekeys non virtual but make
Data::GetDomains and Data::GetSitekeys virtual. Of course in derived classes
like RegExpFilter to get access to members of RegExpFilter::Data we firstly have
to make static_cast to it but it has no overhead in runtime.

https://codereview.adblockplus.org/29333474/diff/29337615/compiled/StringScan...
File compiled/StringScanner.h (right):

https://codereview.adblockplus.org/29333474/diff/29337615/compiled/StringScan...
compiled/StringScanner.h:47: String::size_type oldPos = mPos;
What about reusing of skipOne?

bool skipped = false;
while(skipped = skipOne(ch));
return skipped;

https://codereview.adblockplus.org/29333474/diff/29345688/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29333474/diff/29345688/.gitignore#newcode3
.gitignore:3: compiled/bindings.cpp.js
I think the paths should start with '/' because they should be relative to the
root of the project.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:75: done = scanner.done();
Should it be in the condition?
while (!(done = scanner.done()))

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:84: String::size_type len = scanner.position() -
start;
If domains is empty string then len will be maximum possible value of
String::size_type.
In this case "domains[start + len - 1]" and "DependentString(domains, start,
len)" will cause access violation.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:187: return !sitekeys && (!domains ||
(*domains)[u""_str]);
JIC, pay attention that we can modify domains if there is no entry with "" key
thus we modify current ActiveFilter from the const method. Is it OK here?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:25: FilterNotifier.triggerListeners(getStringData($0),
filter, $3, $4);\
We need to call filter.delete();
See also
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i....

The rule that the caller must call AddRef for a parameter is a good rule in
addition to
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...

void f(ref_counter* param)
{
  {
    untrusive_ptr(param);
  }
  // if reference counter of param was zero then now param is not nullptr but
object is already deleted.
}

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:48: static void ToLower(DependentString&& str);
I would prefer to have it separate from this class.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:53: OwnedString EMSCRIPTEN_KEEPALIVE Serialize() const;
Could you please add "override", so it's visible that it's overridden virtual
method and compiler also verifies it.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:15: StringMap<Filter*> knownFilters(8192);
Does it add a big overhead to store intrusive_ptr instead of a raw pointer
there?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:15: StringMap<Filter*> knownFilters(8192);
What about having another object which holds knownFilters and has
MakeFilterFromText which is currently static Filter::FromText?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:141: // for anything and would be deleted immediately.
Actually, we should have a convention that when we return a raw pointer the
caller is responsible for freeing it, we should call AddRef above for "return
knownFilter->second;" and GetKnownFilter should also call AddRef.

In addition we could have intrusive_ptr::detach() which returns raw pointer and
sets internal pointer member to nullptr and intrusive_ptr::attach which takes
the ownership without calling of AddRef (of course attach calls ReleaseRef for
the previously held value if it's valid).

BTW, here is one of the case when intrusive_ptr::operator T*() is dangerous.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:147: Filter* Filter::GetKnownFilter(const String& text)
It seems this method is not used, do we really need it?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.h
File compiled/Filter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.h#n...
compiled/Filter.h:19: std::vector<Subscription> subscriptions;
May be it makes sense to remove this comment from here.

Just wonder, why do we need to have several subscriptions as a member of Filter
object? It seems it should be vice versa, subscription should have a set of
filters.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.h#n...
compiled/Filter.h:39: virtual Type GetType() const = 0;
Can we set filter type in constructor and make this method non virtual? It
should reduce the amount of code and it should not actually add any overhead.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/RegExpFilt...
File compiled/RegExpFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/RegExpFilt...
compiled/RegExpFilter.h:77: class RegExpFilter : public ActiveFilter, protected
RegExpFilterData
It seems RegExpFilterData should be a member instead of a base class. See also
https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h#n...
compiled/String.h:5: #include <algorithm>
I think we don't need these headers.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/StringMap....
compiled/StringMap.h:176: resize(mBucketCount << 1);
For the future: We need to take a look at the actual size of the container it
could be useful to grow faster at the beginning and slower starting from a
certain size.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/StringMap....
compiled/StringMap.h:227: {
I have read again about default and value initialization, it looks a bit vague
how "second" member is initialized when it is a scalar, e.g. bool (see
ActiveFilter::mDomains), so I would recommend to add ", second{}" here to be
sure that it's always false.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:1: #pragma once
Why are "-1" at the beginning of compiled/bindings.js?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:1: #pragma once
Why does it not print 'use strict;'?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:1: #pragma once
I don't think that it is worth paying attention to comments regarding style in
this file, I left some of them only for reference.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:11: #include <vector>
What do you think about excluding std classes from the resulting JS completely?
We can still use them to generate bindings.

For EMSCRIPTEN_BINDINGS when PRINT_BINDINGS is not defined we don't need most of
these headers, I expect the result code to be much smaller if we completely
exclude <vector>, <map>, <string>, <exception> from all files. I do believe that
anyway emcc removes not used code from the final result but if we can do it
explicitly it can be even better.

BTW, currently we don't need to include <cstring> and <cstdio>.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:18: namespace bindings_internal
It would be good to have some comment here which explains what is happening and
why we need it. May be it should be somewhere in readme file. As I see it, this
file is devoted to generating the glue code to have an ability to call JS code
converted from C++ from regular JS code because we don't know the names of
methods in the generated code. The process of obtaining of the glue code is (see
"compile" file):
- compile C++ code and bindings with -DPRINT_BINDINGS using emcc
- run the generated JS code to generate the glue code.

It's also important to document somewhere that bindings define new classes which
accept "pointer" to C++ object in the constructor, where "pointer" refers to a
reference to an instance of corresponding JS object from JS generated from C++.

Of course it's all clear from the code but I think that short summary at the
beginning of the file will help to faster get head around it.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:36: static char c;
I think it would be better to call it something like "s_typeIDHelper" and put a
comment about the used trick here, something like the following:

The pointer to it is used as TYPEID.
It's not initialized to any specific value because we don't use the value of it,
thus the type of it (char) is also not important, however we use the pointer to
it (which is implicitly casted to TYPEID which is void*). We can do it because
the compiler instantiates a new static member for each TypeInfo<T>, so we get
different pointer &TypeInfo<T>::c values for different T.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:68: TYPEID pointer_type() const
Actually it might be makes sense to make the method also constexpr.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:92: name[0] = 0;
Although it makes no difference I personally prefer to write '\0' to emphasize
that it's a character.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:433: 
Additional line.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:462: const std::string generatePropertyDescriptor(const
PropertyInfo& property)
I'm not sure that we need the `const` for the return object.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:475: printf("var sizeofString = %i\n", sizeof(String));
What about ';' after "%i"?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:501: function getStringData(str)
What do you think about renaming of this method. Currently it looks like it
returns some pointer to the data of str but actually it returns a new JS string
object.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:505: return String.fromCharCode.apply(String,
HEAP16.slice(pointer, pointer + length));
Shouldn't we use fromCodePoint instead of fromCharCode here?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:512: this._pointer = pointer;
We should call Module._AddRef on stored _pointer here.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:572: static struct BindingsInitializer {\
I think static is not required here.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:649: void (ClassType::*setter)(FieldType) = 0) const
I would rather use nullptr instead of 0.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:700: std::initializer_list<std::pair<ReturnType,const
char*>> list) const
Just wonder, why are no spaces between types for std::pair?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:704:
mapping.push_back(std::pair<int,std::string>(it->first, it->second));
Merely a shorter syntax, here and above we can use
for (const auto& item: container){}

so here it could be
for (const auto& item : list)
  mapping.emplace_back(item.first, item.second);

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:23: {
It would be good to have assert that mRefCount is not zero.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:30: : mRefCount(0)
To tell the truth I'm not sure that the initial value should be zero.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise
pointers to ref_counted and
This comment is incorrect, pointers are equal, so it would be better to remove
it.

JIC, the destructor should be virtual because otherwise the destructor of
derived object will not be called from ref_counted::ReleaseRef.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:38: {
it would be good to have assert that mRefCount is zero.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:139: operator bool() const
It would be also better to make it explicit operator bool.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:144: operator T*() const
I personally don't like this operator because of its danger, it would be safer
to call .get() each time we need raw pointer.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:164: inline bool operator==(const intrusive_ptr<T>& a,
const intrusive_ptr<U>& b)
JIC, these operators don't always work, e.g. with multiple inheritance when base
classes have virtual table. But I think we can leave them as is. May be casting
pointers to "const ref_counted*" before comparison could solve the issue but it
seems these methods are not used anyway now.

https://codereview.adblockplus.org/29333474/diff/29345688/test/filterClasses.js
File test/filterClasses.js (left):

https://codereview.adblockplus.org/29333474/diff/29345688/test/filterClasses....
test/filterClasses.js:1: /*
Why is license header removed?

[sergei, 2016-11-02 09:31:01]

bring it up

[Wladimir Palant, 2016-12-06 10:48:39]

There we go, Patch Set 26.

https://codereview.adblockplus.org/29333474/diff/29337615/compiled/StringScan...
File compiled/StringScanner.h (right):

https://codereview.adblockplus.org/29333474/diff/29337615/compiled/StringScan...
compiled/StringScanner.h:47: String::size_type oldPos = mPos;
On 2016/06/16 21:16:10, sergei wrote:
> What about reusing of skipOne?
> 
> bool skipped = false;
> while(skipped = skipOne(ch));
> return skipped;

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/.gitignore
File .gitignore (right):

https://codereview.adblockplus.org/29333474/diff/29345688/.gitignore#newcode3
.gitignore:3: compiled/bindings.cpp.js
On 2016/06/16 21:16:16, sergei wrote:
> I think the paths should start with '/' because they should be relative to the
> root of the project.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:75: done = scanner.done();
On 2016/06/16 21:16:19, sergei wrote:
> Should it be in the condition?
> while (!(done = scanner.done()))

No, this will change the logic - currently we are always reading one char too
many. StringScanner will give us a fake separator character for that char that
will make sure we process the domain currently "in progress."

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:84: String::size_type len = scanner.position() -
start;
On 2016/06/16 21:16:17, sergei wrote:
> If domains is empty string then len will be maximum possible value of
> String::size_type.

How so? Both scanner.position() and start are 0 here because the only character
we process is the fake separator.

> In this case "domains[start + len - 1]" and "DependentString(domains, start,
> len)" will cause access violation.

It doesn't, this case is actually covered by the unit tests.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:187: return !sitekeys && (!domains ||
(*domains)[u""_str]);
On 2016/06/16 21:16:20, sergei wrote:
> JIC, pay attention that we can modify domains if there is no entry with "" key
> thus we modify current ActiveFilter from the const method. Is it OK here?

Given that ParseDomains() will always set the entry with "" key (last thing it
does) - yes, this is ok.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:25: FilterNotifier.triggerListeners(getStringData($0),
filter, $3, $4);\
On 2016/06/16 21:16:22, sergei wrote:
> We need to call filter.delete();

We don't. This doesn't create a filter instance, merely a new JavaScript wrapper
around an existing instance. Given that this wrapper is being passed as a
parameter to a call there is no reason why we should addref here.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:48: static void ToLower(DependentString&& str);
On 2016/06/16 21:16:27, sergei wrote:
> I would prefer to have it separate from this class.

Ok, String::tolower() then.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:53: OwnedString EMSCRIPTEN_KEEPALIVE Serialize() const;
On 2016/06/16 21:16:23, sergei wrote:
> Could you please add "override", so it's visible that it's overridden virtual
> method and compiler also verifies it.

It's not a virtual method, hence no "override" here. This method is only called
by our JS bindings and there we map it to a specific class. No point wasting
memory on virtual methods.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp
File compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:15: StringMap<Filter*> knownFilters(8192);
On 2016/06/16 21:16:28, sergei wrote:
> What about having another object which holds knownFilters and has
> MakeFilterFromText which is currently static Filter::FromText?

Sure, we can probably do that - but at the moment I'd rather replicate the
existing APIs as much as possible, we'll need significant adjustments to
JavaScript code as it is already.

On 2016/06/16 21:16:31, sergei wrote:
> Does it add a big overhead to store intrusive_ptr instead of a raw pointer
> there?

Overhead is not the point - if we store intrusive_ptr here we will leak the
filter. The idea is that we only store the filter here for as long as it is
being used. Once no references to that filter are remaining (meaning no
references from FilterStorage in particular) and the filter is released it
should be removed from knownFilters (see TODO comment in the destructor below).

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:66: // TODO: This should be removing from knownFilters
I addressed this TODO comment so that we can stop leaking filters below.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:141: // for anything and would be deleted immediately.
On 2016/06/16 21:16:33, sergei wrote:
> Actually, we should have a convention that when we return a raw pointer the
> caller is responsible for freeing it, we should call AddRef above for "return
> knownFilter->second;" and GetKnownFilter should also call AddRef.

Ok, let's implement proper semantics.

> In addition we could have intrusive_ptr::detach() which returns raw pointer
and
> sets internal pointer member to nullptr and intrusive_ptr::attach which takes
> the ownership without calling of AddRef (of course attach calls ReleaseRef for
> the previously held value if it's valid).

Actually, if our convention is that raw pointers are already addref'ed then we
just should never addref them when taking ownership of them. I called the other
method intrusive_ptr::release() for consistency with unique_ptr.

> BTW, here is one of the case when intrusive_ptr::operator T*() is dangerous.

Yes, we have intrusive_ptr::release() now so let's get rid of it.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.cpp...
compiled/Filter.cpp:147: Filter* Filter::GetKnownFilter(const String& text)
On 2016/06/16 21:16:30, sergei wrote:
> It seems this method is not used, do we really need it?

It isn't used in this form of course - the original code was exposing
Filter.knownFilters so you could do `text in Filter.knownFilters` instead. I see
however that no code appears to be doing that, everything is rather calling
Filter.fromText() blindly. So let's remove this method and see how far we get
with it.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.h
File compiled/Filter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.h#n...
compiled/Filter.h:19: std::vector<Subscription> subscriptions;
On 2016/06/16 21:16:36, sergei wrote:
> Just wonder, why do we need to have several subscriptions as a member of
Filter
> object? It seems it should be vice versa, subscription should have a set of
> filters.

This is merely a reverse mapping meant to speed up lookups. E.g. when a filter
is added it's important to know what subscriptions it belongs to and whether
these are enabled or disabled.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/Filter.h#n...
compiled/Filter.h:39: virtual Type GetType() const = 0;
On 2016/06/16 21:16:34, sergei wrote:
> Can we set filter type in constructor and make this method non virtual? It
> should reduce the amount of code and it should not actually add any overhead.

Actually, if we do that then we can just read out the filter type member from
JavaScript - this will save some overhead. I implemented it this way now.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/RegExpFilt...
File compiled/RegExpFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/RegExpFilt...
compiled/RegExpFilter.h:77: class RegExpFilter : public ActiveFilter, protected
RegExpFilterData
On 2016/06/16 21:16:39, sergei wrote:
> It seems RegExpFilterData should be a member instead of a base class. See also
>
https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h#n...
compiled/String.h:5: #include <algorithm>
On 2016/06/16 21:16:40, sergei wrote:
> I think we don't need these headers.

It compiles without but only because debug.h will pull in a bunch of headers. We
need <algorithm> for std::min(), <cstring> for std::memcpy(), <cstddef> for
size_t.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/StringMap....
compiled/StringMap.h:176: resize(mBucketCount << 1);
On 2016/06/16 21:16:42, sergei wrote:
> For the future: We need to take a look at the actual size of the container it
> could be useful to grow faster at the beginning and slower starting from a
> certain size.

I already chose a fairly good initial size but this can always be optimized of
course.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/StringMap....
compiled/StringMap.h:227: {
On 2016/06/16 21:16:43, sergei wrote:
> I have read again about default and value initialization, it looks a bit vague
> how "second" member is initialized when it is a scalar, e.g. bool (see
> ActiveFilter::mDomains), so I would recommend to add ", second{}" here to be
> sure that it's always false.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:1: #pragma once
On 2016/06/16 21:16:57, sergei wrote:
> Why are "-1" at the beginning of compiled/bindings.js?

That's output from an abort() call not being suppressed. It might be that you
are using an Emscripten version that is different from 1.35.0 that this has been
tested against. The function name resolving here relies on Emscripten internals
so it won't just work against any Emscripten version.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:1: #pragma once
On 2016/06/16 21:16:53, sergei wrote:
> Why does it not print 'use strict;'?

The bindings are only a small part of the resulting file. If we want "use
strict" it should be added to shell.js which goes at the very top. I added it
but it doesn't help much with generated code.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:11: #include <vector>
On 2016/06/16 21:16:50, sergei wrote:
> For EMSCRIPTEN_BINDINGS when PRINT_BINDINGS is not defined we don't need most
of
> these headers, I expect the result code to be much smaller if we completely
> exclude <vector>, <map>, <string>, <exception> from all files.

Headers don't result in any code, they are merely definitions. If anything, this
will marginally speed up the build but I don't think that we are at a stage
where we should put effort into that.

> BTW, currently we don't need to include <cstring> and <cstdio>.

<cstdio> is necessary for puts(). As to <cstring>, this indeed appears to be
unused.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:18: namespace bindings_internal
On 2016/06/16 21:17:05, sergei wrote:
> It would be good to have some comment here which explains what is happening
and
> why we need it.

With the approach changing with each iteration, I wasn't in a hurry to write
documentation of course. But it seems that the current approach is close to what
we will end up with so I'll write some documentation.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:36: static char c;
On 2016/06/16 21:17:10, sergei wrote:
> I think it would be better to call it something like "s_typeIDHelper" and put
a
> comment about the used trick here, something like the following:
> 
> The pointer to it is used as TYPEID.
> It's not initialized to any specific value because we don't use the value of
it,
> thus the type of it (char) is also not important, however we use the pointer
to
> it (which is implicitly casted to TYPEID which is void*). We can do it because
> the compiler instantiates a new static member for each TypeInfo<T>, so we get
> different pointer &TypeInfo<T>::c values for different T.
> 

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:68: TYPEID pointer_type() const
On 2016/06/16 21:16:51, sergei wrote:
> Actually it might be makes sense to make the method also constexpr.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:92: name[0] = 0;
On 2016/06/16 21:17:02, sergei wrote:
> Although it makes no difference I personally prefer to write '\0' to emphasize
> that it's a character.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:433: 
On 2016/06/16 21:16:45, sergei wrote:
> Additional line.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:462: const std::string generatePropertyDescriptor(const
PropertyInfo& property)
On 2016/06/16 21:16:56, sergei wrote:
> I'm not sure that we need the `const` for the return object.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:475: printf("var sizeofString = %i\n", sizeof(String));
On 2016/06/16 21:17:16, sergei wrote:
> What about ';' after "%i"?

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:501: function getStringData(str)
On 2016/06/16 21:17:15, sergei wrote:
> What do you think about renaming of this method. Currently it looks like it
> returns some pointer to the data of str but actually it returns a new JS
string
> object.

Ok, readString() it is - hopefully more obvious.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:505: return String.fromCharCode.apply(String,
HEAP16.slice(pointer, pointer + length));
On 2016/06/16 21:16:46, sergei wrote:
> Shouldn't we use fromCodePoint instead of fromCharCode here?

No, we shouldn't - we are using 16-bit characters. Codepoints can be a
combination of multiple 16-bit characters.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:512: this._pointer = pointer;
On 2016/06/16 21:17:11, sergei wrote:
> We should call Module._AddRef on stored _pointer here.

No, we shouldn't - that's a raw pointer we got here, it should already be
addref'ed.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:572: static struct BindingsInitializer {\
On 2016/06/16 21:16:54, sergei wrote:
> I think static is not required here.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:649: void (ClassType::*setter)(FieldType) = 0) const
On 2016/06/16 21:17:07, sergei wrote:
> I would rather use nullptr instead of 0.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:700: std::initializer_list<std::pair<ReturnType,const
char*>> list) const
On 2016/06/16 21:17:04, sergei wrote:
> Just wonder, why are no spaces between types for std::pair?

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/bindings.i...
compiled/bindings.ipp:704:
mapping.push_back(std::pair<int,std::string>(it->first, it->second));
On 2016/06/16 21:16:59, sergei wrote:
> Merely a shorter syntax, here and above we can use
> for (const auto& item: container){}
> 
> so here it could be
> for (const auto& item : list)
>   mapping.emplace_back(item.first, item.second);

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:23: {
On 2016/06/16 21:17:29, sergei wrote:
> It would be good to have assert that mRefCount is not zero.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:30: : mRefCount(0)
On 2016/06/16 21:17:24, sergei wrote:
> To tell the truth I'm not sure that the initial value should be zero.

Yes, I changed that already along with the general ref counting changes.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise
pointers to ref_counted and
On 2016/06/16 21:17:18, sergei wrote:
> This comment is incorrect, pointers are equal, so it would be better to remove
> it.

Pointers are equal if you compare them in C++. However, if you convert them to
an integer, a Filter* and a ref_counted* pointer to the same object don't have
the same value. This is actually problematic for our JavaScript bindings that
don't know how to convert pointers. I tried improving the comment in order to
make the issue more obvious.

> JIC, the destructor should be virtual because otherwise the destructor of
> derived object will not be called from ref_counted::ReleaseRef.

Even though the constructor of the derived object is virtual?

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:38: {
On 2016/06/16 21:17:20, sergei wrote:
> it would be good to have assert that mRefCount is zero.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:139: operator bool() const
On 2016/06/16 21:17:27, sergei wrote:
> It would be also better to make it explicit operator bool.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:144: operator T*() const
On 2016/06/16 21:17:22, sergei wrote:
> I personally don't like this operator because of its danger, it would be safer
> to call .get() each time we need raw pointer.

Done.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:164: inline bool operator==(const intrusive_ptr<T>& a,
const intrusive_ptr<U>& b)
On 2016/06/16 21:17:21, sergei wrote:
> JIC, these operators don't always work, e.g. with multiple inheritance when
base
> classes have virtual table. But I think we can leave them as is. May be
casting
> pointers to "const ref_counted*" before comparison could solve the issue but
it
> seems these methods are not used anyway now.

Won't C++ cast these pointers automatically in order to compare them?

https://codereview.adblockplus.org/29333474/diff/29345688/test/filterClasses.js
File test/filterClasses.js (left):

https://codereview.adblockplus.org/29333474/diff/29345688/test/filterClasses....
test/filterClasses.js:1: /*
On 2016/06/16 21:17:30, sergei wrote:
> Why is license header removed?

It rather wasn't there in the first place. But I updated unit tests and this
meant adding license headers in particular.

[sergei, 2017-01-10 15:58:19]

next portion of comments.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:75: done = scanner.done();
On 2016/12/06 10:47:05, Wladimir Palant wrote:
> On 2016/06/16 21:16:19, sergei wrote:
> > Should it be in the condition?
> > while (!(done = scanner.done()))
> 
> No, this will change the logic - currently we are always reading one char too
> many. StringScanner will give us a fake separator character for that char that
> will make sure we process the domain currently "in progress."

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:84: String::size_type len = scanner.position() -
start;
On 2016/12/06 10:47:06, Wladimir Palant wrote:
> On 2016/06/16 21:16:17, sergei wrote:
> > If domains is empty string then len will be maximum possible value of
> > String::size_type.
> 
> How so? Both scanner.position() and start are 0 here because the only
character
> we process is the fake separator.
> 
> > In this case "domains[start + len - 1]" and "DependentString(domains, start,
> > len)" will cause access violation.
> 
> It doesn't, this case is actually covered by the unit tests.

Acknowledged. The confusing part here was that it's not expected that
StringScanner::next allows mPos to grow after reaching of the end of the string.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
File compiled/ActiveFilter.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilt...
compiled/ActiveFilter.h:53: OwnedString EMSCRIPTEN_KEEPALIVE Serialize() const;
On 2016/12/06 10:47:17, Wladimir Palant wrote:
> On 2016/06/16 21:16:23, sergei wrote:
> > Could you please add "override", so it's visible that it's overridden
virtual
> > method and compiler also verifies it.
> 
> It's not a virtual method, hence no "override" here. This method is only
called
> by our JS bindings and there we map it to a specific class. No point wasting
> memory on virtual methods.

Acknowledged.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise
pointers to ref_counted and
On 2016/12/06 10:48:28, Wladimir Palant wrote:
> On 2016/06/16 21:17:18, sergei wrote:
> > This comment is incorrect, pointers are equal, so it would be better to
remove
> > it.
> 
> Pointers are equal if you compare them in C++. However, if you convert them to
> an integer, a Filter* and a ref_counted* pointer to the same object don't have
> the same value. This is actually problematic for our JavaScript bindings that
> don't know how to convert pointers. I tried improving the comment in order to
> make the issue more obvious.
Pointers in c++ are already numeric values and are not needed to be converted.
I think I see the issue, however I still think that we don't need that comment
because it is confusing and addresses only one case of a well known behavior of
C++ compiler.
Let's say we have class A{int a1;int a2;}; class B{int b1;int b2;}; and class C
: public B, public A{}; In that case layout of object C in memory is
C B int b1;
    int b2;
  A int a1;
    int a2;
in this case &c - static_cast<A*>(&c) = 8 (here and below size of int is 4
bytes), however &c - static_cast<B*>(&c) = 0.
Now let's add a virtual destructor for A and keep the same inheritance order.
The layout of C is
C A vtable with ~A (for 64 platform its size is 8 bytes, here and below)
    int a1;
    int a2;
  B int b1;
    int b2;
now
&c - static_cast<A*>(&c) = 0 (what the comment says), and &c -
static_cast<B*>(&c) = 16.
Now let's add virtual destructor for B. The layout of C is
C B vtable with ~B
    int b1;
    int b2;
  A vtable with ~A
    int a1;
    int a2;
&c - static_cast<A*>(&c) = 16 (comment is again wrong), and &c -
static_cast<B*>(&c) = 0.


> 
> > JIC, the destructor should be virtual because otherwise the destructor of
> > derived object will not be called from ref_counted::ReleaseRef.
> 
> Even though the constructor of the derived object is virtual?
There is no such concept as virtual constructor in C++.

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:164: inline bool operator==(const intrusive_ptr<T>& a,
const intrusive_ptr<U>& b)
On 2016/12/06 10:48:21, Wladimir Palant wrote:
> On 2016/06/16 21:17:21, sergei wrote:
> > JIC, these operators don't always work, e.g. with multiple inheritance when
> base
> > classes have virtual table. But I think we can leave them as is. May be
> casting
> > pointers to "const ref_counted*" before comparison could solve the issue but
> it
> > seems these methods are not used anyway now.
> 
> Won't C++ cast these pointers automatically in order to compare them?

No.

https://codereview.adblockplus.org/29333474/diff/29366890/compile
File compile (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compile#newcode49
compile:49: env['PATH'] = ':'.join([os.environ['PATH']] + path)
Should not we use os.pathsep instead of ':' here?

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:51: bool done = false;
BTW, it can be initialized to scanner.done() because we should not even enter
into the loop if the string is empty.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:80: (*mDomains)[u""_str] = !hasIncludes;
It would simplify the reading of the code if we had a comment that "" is a flag
indicating whether it's should be applied on all domains. Of course it's clear
from the code but to derive it one has to spend some time.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:128: break;
Should not we rather return Type::UNKNOWN if the filter is in a broken format,
like the end is absent?

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:132: return Type::CSSPROPERTY;
Just in case, do we plan to add any information about -abp-properties to
https://adblockplus.org/filters?

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h
File compiled/Filter.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:27: explicit Filter(Type type, const String& text);
Just observation:
Before C++11 the compiler was allowed to implicitly construct some class only
for one argument case, but starting with C++11 it may also decide to construct a
class when initializer-list like expression is used, e.g. {arg1, arg2, ...},
including also the case with one argument, {arg}. It also seems a bit
complicated how and when a particular constructor is chosen, so, it seems now it
makes sense to use explicit for all constructors just in case and drop it in
rare cases, when it's getting annoying and we are sure it won't conflict with
something else.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:28: virtual ~Filter();
I'm not sure that we need to say virtual here because we already know that the
destructor is virtual because ref_counted has a virtual destructor.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:30: Type mType;
I guess it's a public member only for the sake of performance. In JS it should
be at least two times faster to simply read the member of object than to call a
method which returns the value of the member. Right?

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:43: static EMSCRIPTEN_KEEPALIVE Type GetType(Filter* filter)
It seems we don't need this method anymore.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/RegExpFilt...
File compiled/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/RegExpFilt...
compiled/RegExpFilter.cpp:252: return Type::WHITELIST;
It seems we could simply change type of blocking variable to Filter::Type,
assign the values of Type type and then simply return blocking.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:150: void tolower()
it seems to be consistent it should be to_lower or toLower.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:163: // It seems that calling JS it the easiest solution for
lowercasing
typo: it -> is

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:261: size_type oldLength = length();
It seems it can be simplified to

auto newValue = OwnedString(length() + additionalSize);
if (length() > 0)
  std::memcpy(mBuf, oldBuffer, sizeof(value_type) * oldLength);
*this = std::move(newValue);

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:328: OwnedString& operator=(OwnedString&& str)
I don't know how I missed it before but it's incorrect.
Let's say we have
OwnedString value = u"abc"_str; // it allocates memory (mBuf).
value = std::move(u"new value"_str); // who will delete old mBuf pointing to
"abc"?

I would simply swap members of this and str.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringMap....
compiled/StringMap.h:140: entry_type* entry = &mBuckets[h & mBucketCount - 1];
Could we put "h & (mBucketCount - 1)" to hide compiler warning?

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
File compiled/StringScanner.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
compiled/StringScanner.h:17: bool done()
this method should be const.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
compiled/StringScanner.h:22: String::size_type position()
this method also should be const.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
compiled/StringScanner.h:48: while ((skipped = skipOne(ch)));
Nit: extra parentheses are actually not needed.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h
File compiled/debug.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h#ne...
compiled/debug.h:35: static console_type console;
Why do we need a global static variable of a class all methods of which are
static?

[Wladimir Palant, 2017-03-13 17:36:57]

Addressed comments from Patch Set 26

[Wladimir Palant, 2017-03-13 17:42:40]

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise
pointers to ref_counted and
Ok, I removed the comment here and instead added one in bindings.ipp - that's
where we have the issue, and we should keep it in mind.

https://codereview.adblockplus.org/29333474/diff/29366890/compile
File compile (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compile#newcode49
compile:49: env['PATH'] = ':'.join([os.environ['PATH']] + path)
On 2017/01/10 15:57:27, sergei wrote:
> Should not we use os.pathsep instead of ':' here?

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:51: bool done = false;
On 2017/01/10 15:57:29, sergei wrote:
> BTW, it can be initialized to scanner.done() because we should not even enter
> into the loop if the string is empty.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:80: (*mDomains)[u""_str] = !hasIncludes;
On 2017/01/10 15:57:31, sergei wrote:
> It would simplify the reading of the code if we had a comment that "" is a
flag
> indicating whether it's should be applied on all domains. Of course it's clear
> from the code but to derive it one has to spend some time.

Yes, it's a magic value and we shouldn't have it like that. Looks like the
compiler didn't optimize this properly either, done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ElemHideBa...
File compiled/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:128: break;
On 2017/01/10 15:57:38, sergei wrote:
> Should not we rather return Type::UNKNOWN if the filter is in a broken format,
> like the end is absent?

No, a CSS property filter is an element hiding filter containing
[-abp-properties="..."]. If it doesn't contain that or only part of it then it
is just a regular element hiding filter and we leave it up to the browser to
validate its selector.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/ElemHideBa...
compiled/ElemHideBase.cpp:132: return Type::CSSPROPERTY;
On 2017/01/10 15:57:40, sergei wrote:
> Just in case, do we plan to add any information about -abp-properties to
> https://adblockplus.org/filters

We do, once we don't consider them experimental any more. Right now they come
with big warnings and we don't want to encourage their use.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h
File compiled/Filter.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:27: explicit Filter(Type type, const String& text);
On 2017/01/10 15:57:46, sergei wrote:
> Just observation:
> Before C++11 the compiler was allowed to implicitly construct some class only
> for one argument case, but starting with C++11 it may also decide to construct
a
> class when initializer-list like expression is used, e.g. {arg1, arg2, ...},
> including also the case with one argument, {arg}. It also seems a bit
> complicated how and when a particular constructor is chosen, so, it seems now
it
> makes sense to use explicit for all constructors just in case and drop it in
> rare cases, when it's getting annoying and we are sure it won't conflict with
> something else.

I see. Here it rather seems to be a left-over from a previous version but I'm
fine with making all constructors explicit. That's a lot of them however.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:28: virtual ~Filter();
On 2017/01/10 15:57:48, sergei wrote:
> I'm not sure that we need to say virtual here because we already know that the
> destructor is virtual because ref_counted has a virtual destructor.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:30: Type mType;
On 2017/01/10 15:57:44, sergei wrote:
> I guess it's a public member only for the sake of performance. In JS it should
> be at least two times faster to simply read the member of object than to call
a
> method which returns the value of the member. Right?

Yes, we were calling a function originally in order to differentiate between
subclasses. Reading out a value from memory is considerably faster and it also
happens to be simpler to implement in our bindings.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/Filter.h#n...
compiled/Filter.h:43: static EMSCRIPTEN_KEEPALIVE Type GetType(Filter* filter)
On 2017/01/10 15:57:42, sergei wrote:
> It seems we don't need this method anymore.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/RegExpFilt...
File compiled/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/RegExpFilt...
compiled/RegExpFilter.cpp:252: return Type::WHITELIST;
On 2017/01/10 15:57:50, sergei wrote:
> It seems we could simply change type of blocking variable to Filter::Type,
> assign the values of Type type and then simply return blocking.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:150: void tolower()
On 2017/01/10 15:58:02, sergei wrote:
> it seems to be consistent it should be to_lower or toLower.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:163: // It seems that calling JS it the easiest solution for
lowercasing
On 2017/01/10 15:57:54, sergei wrote:
> typo: it -> is

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:261: size_type oldLength = length();
On 2017/01/10 15:57:56, sergei wrote:
> It seems it can be simplified to
> 
> auto newValue = OwnedString(length() + additionalSize);
> if (length() > 0)
>   std::memcpy(mBuf, oldBuffer, sizeof(value_type) * oldLength);
> *this = std::move(newValue);

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/String.h#n...
compiled/String.h:328: OwnedString& operator=(OwnedString&& str)
On 2017/01/10 15:57:52, sergei wrote:
> I don't know how I missed it before but it's incorrect.
> Let's say we have
> OwnedString value = u"abc"_str; // it allocates memory (mBuf).
> value = std::move(u"new value"_str); // who will delete old mBuf pointing to
> "abc"?
> 
> I would simply swap members of this and str.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringMap....
compiled/StringMap.h:140: entry_type* entry = &mBuckets[h & mBucketCount - 1];
On 2017/01/10 15:58:04, sergei wrote:
> Could we put "h & (mBucketCount - 1)" to hide compiler warning?

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
File compiled/StringScanner.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
compiled/StringScanner.h:17: bool done()
On 2017/01/10 15:58:08, sergei wrote:
> this method should be const.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
compiled/StringScanner.h:22: String::size_type position()
On 2017/01/10 15:58:10, sergei wrote:
> this method also should be const.

Done.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/StringScan...
compiled/StringScanner.h:48: while ((skipped = skipOne(ch)));
On 2017/01/10 15:58:06, sergei wrote:
> Nit: extra parentheses are actually not needed.

They are, in order to avoid a compiler warning - they indicate that the
assignment is intentional.

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h
File compiled/debug.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h#ne...
compiled/debug.h:35: static console_type console;
On 2017/01/10 15:58:12, sergei wrote:
> Why do we need a global static variable of a class all methods of which are
> static?

Just so that one can call `console.log()` the way you would do in JavaScript.

[Wladimir Palant, 2017-03-14 10:23:17]

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_...
compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise
pointers to ref_counted and
On 2017/03/13 17:41:46, Wladimir Palant wrote:
> Ok, I removed the comment here and instead added one in bindings.ipp - that's
> where we have the issue, and we should keep it in mind.

Actually, it is better to remove the footgun instead of commenting on it. I
realized that this is possible, with Patch Set 28 we will handle offsets
correctly.

[hub, 2017-03-20 16:21:57]

Nothing holding back landing really, but a couple of comments about eventual
Windows compatibility glitches.

If this matter they can be done as a follow up shortly thereafter.

https://codereview.adblockplus.org/29333474/diff/29383674/compile
File compile (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compile#newcode19
compile:19: COMPILER_OUTPUT = './lib/compiled.js'
Shouldn't you use os.path.join() here?

https://codereview.adblockplus.org/29333474/diff/29383674/compile#newcode41
compile:41: '/bin/bash', '-c', os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh')])
Will that even work on Windows? If not, we should make sure it does in a
follow-up.

[sergei, 2017-03-20 17:07:27]

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h
File compiled/debug.h (right):

https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h#ne...
compiled/debug.h:35: static console_type console;
On 2017/03/13 17:42:32, Wladimir Palant wrote:
> On 2017/01/10 15:58:12, sergei wrote:
> > Why do we need a global static variable of a class all methods of which are
> > static?
> 
> Just so that one can call `console.log()` the way you would do in JavaScript.

`console.log()` calls a method of `console` object, it's not required to have
these methods to be static. Though, for me it's fine to have the class with
static methods but I think that we don't need an instance of it (per each
translation unit), it should work if we simply rename console_type to console.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:6: static const DependentString
DEFAULT_DOMAIN(u""_str);
What about putting it into anonymous namespace?
BTW, I would remove static here because it's not referenced anywhere else.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/String.h#n...
compiled/String.h:191: DependentString(String& str, size_type pos = 0, size_type
len = npos)
Why have explicit keywords been removed here?

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/String.h#n...
compiled/String.h:253: auto newValue = OwnedString(length() + additionalSize);
I know, it's a copy from my code but
OwnedString newValue(length() + additionalSize);
looks better, IMO.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/StringScan...
File compiled/StringScanner.h (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/StringScan...
compiled/StringScanner.h:17: const bool done()
It should be `bool done() const`.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/StringScan...
compiled/StringScanner.h:22: const String::size_type position()
it should be `String::size_type position() const`.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/bindings.i...
compiled/bindings.ipp:280: int ref_counted_offset;
Can we use ptrdiff_t for ref_counted_offset?

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/bindings.i...
compiled/bindings.ipp:650:
reinterpret_cast<int>(dynamic_cast<ref_counted*>(ptr)) -
Could we please use static_cast instead of dynamic_cast here because I find it
very unreliable because I expect that dynamic_cast uses run time info, in
particular it likely reads vtable (maybe a value at some negative offset) which
is absent here.

I would say it's still a hack and so far the offset is always zero, so what do
you think about having of some warning when the offset is not zero to come back
and check that it still works correctly.

BTW, in general I think this code does not always work, e.g. with some virtual
inheritance  but I think it will never our case and I think static_cast will do
its work in this case.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/intrusive_...
compiled/intrusive_ptr.h:63: explicit intrusive_ptr(const intrusive_ptr& other)
I think that copy and move constructors should not explicit.

[sergei, 2017-03-20 17:09:08]

I think it may be pushed, I also agree that whatever can be fixed in following
commits.

[Wladimir Palant, 2017-03-21 10:10:25]

https://codereview.adblockplus.org/29333474/diff/29383674/compile
File compile (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compile#newcode19
compile:19: COMPILER_OUTPUT = './lib/compiled.js'
On 2017/03/20 16:21:49, hub wrote:
> Shouldn't you use os.path.join() here?

This compile script is very much a temporary solution, and not using
os.path.join() isn't even the biggest issue here. While I doubt that we will
keep it in the long term, there is nothing wrong with making it easier to use
for now. I filed https://issues.adblockplus.org/ticket/5020 on this.

https://codereview.adblockplus.org/29333474/diff/29383674/compile#newcode41
compile:41: '/bin/bash', '-c', os.path.join(EMSCRIPTEN_PATH, 'emsdk_env.sh')])
On 2017/03/20 16:21:49, hub wrote:
> Will that even work on Windows? If not, we should make sure it does in a
> follow-up.

No, of course it won't. So far I didn't even try installing Emscripten on
Windows. This is also part of https://issues.adblockplus.org/ticket/5020.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/ActiveFilt...
File compiled/ActiveFilter.cpp (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/ActiveFilt...
compiled/ActiveFilter.cpp:6: static const DependentString
DEFAULT_DOMAIN(u""_str);
On 2017/03/20 17:07:02, sergei wrote:
> What about putting it into anonymous namespace?
> BTW, I would remove static here because it's not referenced anywhere else.

Done.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/String.h#n...
compiled/String.h:191: DependentString(String& str, size_type pos = 0, size_type
len = npos)
On 2017/03/20 17:07:06, sergei wrote:
> Why have explicit keywords been removed here?

Done.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/String.h#n...
compiled/String.h:253: auto newValue = OwnedString(length() + additionalSize);
On 2017/03/20 17:07:04, sergei wrote:
> I know, it's a copy from my code but
> OwnedString newValue(length() + additionalSize);
> looks better, IMO.

Done.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/StringScan...
File compiled/StringScanner.h (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/StringScan...
compiled/StringScanner.h:17: const bool done()
On 2017/03/20 17:07:12, sergei wrote:
> It should be `bool done() const`.

Done.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/StringScan...
compiled/StringScanner.h:22: const String::size_type position()
On 2017/03/20 17:07:10, sergei wrote:
> it should be `String::size_type position() const`.

Done.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/bindings.i...
compiled/bindings.ipp:280: int ref_counted_offset;
On 2017/03/20 17:07:18, sergei wrote:
> Can we use ptrdiff_t for ref_counted_offset?

Done.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/bindings.i...
compiled/bindings.ipp:650:
reinterpret_cast<int>(dynamic_cast<ref_counted*>(ptr)) -
On 2017/03/20 17:07:15, sergei wrote:
> Could we please use static_cast instead of dynamic_cast here because I find it
> very unreliable because I expect that dynamic_cast uses run time info, in
> particular it likely reads vtable (maybe a value at some negative offset)
which
> is absent here.

Done.

> I would say it's still a hack and so far the offset is always zero, so what do
> you think about having of some warning when the offset is not zero to come
back
> and check that it still works correctly.

I did test this code of course by changing the inheritance structure in such a
way that the offset is no longer zero.

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/intrusive_...
File compiled/intrusive_ptr.h (right):

https://codereview.adblockplus.org/29333474/diff/29383674/compiled/intrusive_...
compiled/intrusive_ptr.h:63: explicit intrusive_ptr(const intrusive_ptr& other)
On 2017/03/20 17:07:20, sergei wrote:
> I think that copy and move constructors should not explicit.

Done.

[sergei, 2017-03-21 11:36:39]

LGTM