Issue 6241 - Allow a native build.

Issue 6241 - Allow a native build.

[sergei, 2018-01-11 08:54:07]

Good, I'm mainly concerned about ID of regular expressions and from_string.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cpp
File compiled/library.cpp (right):

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:33: {
stubs are OK for present.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:42: std::vector<std::unique_ptr<std::wregex>> mRegexPool;
BTW, this code is not thread safe, we should be very careful with that because
libadblockplus is multithreaded.
I'm not sure whether we should already add some serializing the access mechanism
because there is plenty of other places like this, so maybe it should be
addressed at another level or uniformly but later.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:44: std::wstring from_string(const String& str)
What about putting this method into some utils.h (possible cpp) file which is
used in the native builds because it seems we will need it more than only here?
BTW, IMO it should be named, something like StdWStringFromString.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:48: s.push_back(str[idx]);
std::string has the constructor accepting a pointer to an array of characters
and its size, IMO it would be better to use it here, e.g.
return std::wstring(str.data(), str.length());

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:67: mRegexPool[id].reset();
What about using of std::list and its iterator as ID, one can remove an element
without invalidation of other iterators. BTW in this case there will not be the
necessity in a smart pointer.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:75: return false;
Could please add an assert here because it should never happen that id is
invalid.

[hub, 2018-01-11 20:41:28]

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cpp
File compiled/library.cpp (right):

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:42: std::vector<std::unique_ptr<std::wregex>> mRegexPool;
On 2018/01/11 08:54:06, sergei wrote:
> BTW, this code is not thread safe, we should be very careful with that because
> libadblockplus is multithreaded.
> I'm not sure whether we should already add some serializing the access
mechanism
> because there is plenty of other places like this, so maybe it should be
> addressed at another level or uniformly but later.

Done.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:44: std::wstring from_string(const String& str)
On 2018/01/11 08:54:07, sergei wrote:
> What about putting this method into some utils.h (possible cpp) file which is
> used in the native builds because it seems we will need it more than only
here?
> BTW, IMO it should be named, something like StdWStringFromString.

Done.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:48: s.push_back(str[idx]);
On 2018/01/11 08:54:07, sergei wrote:
> std::string has the constructor accepting a pointer to an array of characters
> and its size, IMO it would be better to use it here, e.g.
> return std::wstring(str.data(), str.length());

std::wstring::value_type is a wchar_t which is 32-bits (Windows might be the
exception), so this can't work.

Also we can't use std::u16string as char16_t doesn't have a std::ctype
implementation provided.

However std::wstring s(str.data(), str.data() + str.length()) will work
(iterator range).

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:67: mRegexPool[id].reset();
On 2018/01/11 08:54:07, sergei wrote:
> What about using of std::list and its iterator as ID, one can remove an
element
> without invalidation of other iterators. BTW in this case there will not be
the
> necessity in a smart pointer.

How do you know if the id is valid? erase() invalidate the iterator of the
erased item, and there is no way to check if it is derefenceable.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:75: return false;
On 2018/01/11 08:54:07, sergei wrote:
> Could please add an assert here because it should never happen that id is
> invalid.

Done.

[sergei, 2018-01-12 09:58:12]

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cpp
File compiled/library.cpp (right):

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:48: s.push_back(str[idx]);
On 2018/01/11 20:41:27, hub wrote:
> On 2018/01/11 08:54:07, sergei wrote:
> > std::string has the constructor accepting a pointer to an array of
characters
> > and its size, IMO it would be better to use it here, e.g.
> > return std::wstring(str.data(), str.length());
> 
> std::wstring::value_type is a wchar_t which is 32-bits (Windows might be the
> exception), so this can't work.
> 
> Also we can't use std::u16string as char16_t doesn't have a std::ctype
> implementation provided.
> 
> However std::wstring s(str.data(), str.data() + str.length()) will work
> (iterator range).

Acknowledged.

https://codereview.adblockplus.org/29661608/diff/29661609/compiled/library.cp...
compiled/library.cpp:67: mRegexPool[id].reset();
On 2018/01/11 20:41:27, hub wrote:
> On 2018/01/11 08:54:07, sergei wrote:
> > What about using of std::list and its iterator as ID, one can remove an
> element
> > without invalidation of other iterators. BTW in this case there will not be
> the
> > necessity in a smart pointer.
> 
> How do you know if the id is valid? erase() invalidate the iterator of the
> erased item, and there is no way to check if it is derefenceable.

IMO there is no necessity in it, as soon as the regexp is removed, its ID is not
supposed to be used, so it's fine if we just throw an exception or the app
crashes :).

https://codereview.adblockplus.org/29661608/diff/29663576/compiled/library.cpp
File compiled/library.cpp (right):

https://codereview.adblockplus.org/29661608/diff/29663576/compiled/library.cp...
compiled/library.cpp:45: std::vector<std::unique_ptr<std::wregex>> regexPool;
I missed it first time, but what is the whole point of it? Why do we need such
pool? GenerateRegExp should just return std::wregex, DeleteRegExp should do
nothing and TestRegExp should use std::wregex.

[hub, 2018-01-12 14:33:53]

https://codereview.adblockplus.org/29661608/diff/29663576/compiled/library.cpp
File compiled/library.cpp (right):

https://codereview.adblockplus.org/29661608/diff/29663576/compiled/library.cp...
compiled/library.cpp:45: std::vector<std::unique_ptr<std::wregex>> regexPool;
On 2018/01/12 09:58:12, sergei wrote:
> I missed it first time, but what is the whole point of it? Why do we need such
> pool? GenerateRegExp should just return std::wregex, DeleteRegExp should do
> nothing and TestRegExp should use std::wregex.

This is because of the original code and bindings in the emscripten branch
(library.js) ; this is meant to be a drop in replacement.

(I'm not a fan of it either)

[sergei, 2018-01-15 10:50:42]

I have files a couple of issues regarding this codereview.
https://issues.adblockplus.org/ticket/6266
https://issues.adblockplus.org/ticket/6267

LGTM.