Issue 768 - Switch from TR1 to C++11

Note that Linux is still using libstdc++, also it uses -std=c++0x for compatibility with GCC versions before 4.7.

COLLABORATOR=sergei@adblockplus.org

[Wladimir Palant, 2014-07-11 14:29:11]

http://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/incl...
include/AdblockPlus/FilterEngine.h:91: void ForceUpdateCheck(UpdaterCallback
callback);
Actually, there is one more actual code change here - libc++ wouldn't let me
assign a default value here.

[René Jeschke, 2014-08-13 11:11:30]

As this is more or less only replacing 'std::tr1::' by 'std::': 

LGTM.

[Felix Dahlke, 2014-09-05 17:56:09]

I didn't review it yet, but I will.

One remark: I think we should point out the C++11 requirement in the README,
something like:

"You need a C++11 compatible compiler to use libadblockplus. Please [create an
issue](...) if you need a C++98 compatible interface."

[sergei, 2014-11-06 10:56:38]

Works on MSVS2010

[sergei, 2015-01-06 13:51:39]

If it works and the everything is fine with gyp files, LGTM

http://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/incl...
include/AdblockPlus/FilterEngine.h:91: void ForceUpdateCheck(UpdaterCallback
callback);
On 2014/07/11 14:29:11, Wladimir Palant wrote:
> Actually, there is one more actual code change here - libc++ wouldn't let me
> assign a default value here.

Actually, when do we need a default value here?

Regarding the code, it's surprising that it was working with ` = 0`. Have you
tried ` = UpdaterCallback()`?

[Felix Dahlke, 2015-08-04 11:21:50]

I suppose this needs rebasing - there is no .hgsubstate in libadblockplus
anymore, e.g. But apart from that, and my comments (also my old one:
https://codereview.adblockplus.org/5163715573841920/#msg4), it looks good to me,
ASSUMING:

1. This compiles in MSVS 2010 (Sergei said so, so we're probably fine)
2. It compiles for all the Android versions we currently support (seems like
Wladimir actually tested this, from the looks of it)

https://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/.hg...
File .hgsubstate (right):

https://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/.hg...
.hgsubstate:2: bd26b148913a32213c37bd1b3a38cdb98ce059cb third_party/googletest
Could you add something to the review description and commit message about why a
googletest update is necessary as a part of this?

https://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/inc...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/inc...
include/AdblockPlus/FilterEngine.h:91: void ForceUpdateCheck(UpdaterCallback
callback);
On 2015/01/06 13:51:39, sergei wrote:
> On 2014/07/11 14:29:11, Wladimir Palant wrote:
> > Actually, there is one more actual code change here - libc++ wouldn't let me
> > assign a default value here.
> 
> Actually, when do we need a default value here?
> 
> Regarding the code, it's surprising that it was working with ` = 0`. Have you
> tried ` = UpdaterCallback()`?

Probably some subtle differences between std::function and std::tr1::function?
Also not entirely sure if this would have worked, we're not using it anywhere.
Let's just get rid of the default until we have a use case for it, I'd say.

[sergei, 2015-08-05 10:40:11]

https://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/.hg...
File .hgsubstate (right):

https://codereview.adblockplus.org/5163715573841920/diff/5629499534213120/.hg...
.hgsubstate:2: bd26b148913a32213c37bd1b3a38cdb98ce059cb third_party/googletest
On 2015/08/04 11:21:49, Felix Dahlke wrote:
> Could you add something to the review description and commit message about why
a
> googletest update is necessary as a part of this?

It seems not necessary here, the change in gyp file is enough. I'm excluding it
from this codereview. Let's do it as another issue, because updating of it can
even drop the support of some old compilers.

[Wladimir Palant, 2015-08-05 11:19:40]

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md
File README.md (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:56: You need a C\+\+11 compatible compiler to build libadblockplus. If
you need a C++98 compatible interface or you have a questions about another
comiler please [create an
Nit: Please break that line at 80 characters.

Nit: + has no special meaning in Markdown, no need to escape it.

Typo: "comiler" => "compiler"

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:57: issue](https://issues.adblockplus.org/).
Will we actually create a C++98 compatible interface for somebody who creates an
issue? I don't think so, and IMHO we shouldn't suggest here that we will.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:61: * std::function
IMHO, this list is bound to get out of date. Yes, right now we are only using
these two features - because we only had headers for them before. Now people
will start using other C++11 features as well however and they won't think about
updating this list.

Why have it here at all? If this is meant to be normative then it belongs in our
coding style document.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/JsValue.h:24: #include "V8ValueHolder.h"
You are assuming that <vector> will include <memory> internally. IMHO that's a
bad idea, you should just spell out your requirements explicitly without making
any assumptions.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/Notification.h:23: 
You are assuming that <vector> will include <memory> internally. IMHO that's a
bad idea, you should just spell out your requirements explicitly without making
any assumptions.

[Felix Dahlke, 2015-08-05 21:28:06]

Looks good. With my late night nits and Wladimir's comments addressed, it should
be fine.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md
File README.md (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:57: issue](https://issues.adblockplus.org/).
On 2015/08/05 11:19:39, Wladimir Palant wrote:
> Will we actually create a C++98 compatible interface for somebody who creates
an
> issue? I don't think so, and IMHO we shouldn't suggest here that we will.

Fair enough, probably not. I suppose anyone who really wants that is going to
figure out that they can create an issue anyway.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:61: * std::function
On 2015/08/05 11:19:39, Wladimir Palant wrote:
> IMHO, this list is bound to get out of date. Yes, right now we are only using
> these two features - because we only had headers for them before. Now people
> will start using other C++11 features as well however and they won't think
about
> updating this list.
> 
> Why have it here at all? If this is meant to be normative then it belongs in
our
> coding style document.

I was about to object but, rethinking it, I suppose a simple `grep` should
answer this, with no chance of getting out of sync with the code base.
Additionally, if we really do test with the compilers below, it'll be fine
anyway.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:68: 
Nit: Just one new line between sections, as elsewhere here?

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/DefaultFileSystem.h:42: std::shared_ptr<std::istream> data);
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:40: public
std::enable_shared_from_this<Filter>
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:167: typedef std::function<void(const
std::string&)>
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:174: typedef std::function<void(const
std::string&)>
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/Default...
File src/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/Default...
src/DefaultFileSystem.cpp:79: std::shared_ptr<std::istream> data)
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/FilterE...
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/FilterE...
src/FilterEngine.cpp:289: std::placeholders::_1));
Nit: This was previously indented on column (i.e. aligned with the argument in
the line above.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/FilterE...
src/FilterEngine.cpp:386: std::placeholders::_1));
Nit: This was previously indented on column (i.e. aligned with the argument in
the line above.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/test/BaseJs...
File test/BaseJsTest.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/test/BaseJs...
test/BaseJsTest.h:44: std::shared_ptr<std::istream> content)
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/test/BaseJs...
test/BaseJsTest.h:94: std::shared_ptr<std::istream> content)
Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can be
in one line again.

[sergei, 2015-08-06 07:35:31]

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md
File README.md (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:56: You need a C\+\+11 compatible compiler to build libadblockplus. If
you need a C++98 compatible interface or you have a questions about another
comiler please [create an
On 2015/08/05 11:19:39, Wladimir Palant wrote:
> Nit: Please break that line at 80 characters.
> 
> Nit: + has no special meaning in Markdown, no need to escape it.
> 
> Typo: "comiler" => "compiler"

Thanks.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:57: issue](https://issues.adblockplus.org/).
On 2015/08/05 21:28:04, Felix Dahlke wrote:
> On 2015/08/05 11:19:39, Wladimir Palant wrote:
> > Will we actually create a C++98 compatible interface for somebody who
creates
> an
> > issue? I don't think so, and IMHO we shouldn't suggest here that we will.
> 
> Fair enough, probably not. I suppose anyone who really wants that is going to
> figure out that they can create an issue anyway.

Removed mentioning of C++98.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/README.md#n...
README.md:61: * std::function
On 2015/08/05 21:28:03, Felix Dahlke wrote:
> On 2015/08/05 11:19:39, Wladimir Palant wrote:
> > IMHO, this list is bound to get out of date. Yes, right now we are only
using
> > these two features - because we only had headers for them before. Now people
> > will start using other C++11 features as well however and they won't think
> about
> > updating this list.
> > 
> > Why have it here at all? If this is meant to be normative then it belongs in
> our
> > coding style document.
> 
> I was about to object but, rethinking it, I suppose a simple `grep` should
> answer this, with no chance of getting out of sync with the code base.
> Additionally, if we really do test with the compilers below, it'll be fine
> anyway.

Agree that there is a big chance to get it out of sync.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/DefaultFileSystem.h:42: std::shared_ptr<std::istream> data);
On 2015/08/05 21:28:04, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Fixed and the line below.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:167: typedef std::function<void(const
std::string&)>
On 2015/08/05 21:28:04, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Fixed.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:174: typedef std::function<void(const
std::string&)>
On 2015/08/05 21:28:04, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Fixed.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/JsValue.h:24: #include "V8ValueHolder.h"
On 2015/08/05 11:19:39, Wladimir Palant wrote:
> You are assuming that <vector> will include <memory> internally. IMHO that's a
> bad idea, you should just spell out your requirements explicitly without
making
> any assumptions.

<vector> does not include <memory>, it's included by "V8ValueHolder.h". Anyway I
find it a good idea to include <memory> here now.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/Notification.h:23: 
On 2015/08/05 11:19:40, Wladimir Palant wrote:
> You are assuming that <vector> will include <memory> internally. IMHO that's a
> bad idea, you should just spell out your requirements explicitly without
making
> any assumptions.

Here the same, <memory> is not included by <vector>, it's included in cpp file
by another header before including of this header. Anyway, it's always good to
have all required headers to be included in the header.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/test/BaseJs...
File test/BaseJsTest.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/test/BaseJs...
test/BaseJsTest.h:44: std::shared_ptr<std::istream> content)
On 2015/08/05 21:28:05, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Done.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/test/BaseJs...
test/BaseJsTest.h:94: std::shared_ptr<std::istream> content)
On 2015/08/05 21:28:05, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Done.

[Wladimir Palant, 2015-08-06 12:24:58]

From what I can tell, only two comments still need to be addressed.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:40: public
std::enable_shared_from_this<Filter>
On 2015/08/05 21:28:04, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Sergei didn't address this nit and I think he is right - wrapping here isn't
because of the line length limit. See
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...
- "When deriving from more than one class, put each on its own line."

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/JsValue.h:24: #include "V8ValueHolder.h"
On 2015/08/06 07:35:30, sergei wrote:
> <vector> does not include <memory>, it's included by "V8ValueHolder.h". Anyway
I
> find it a good idea to include <memory> here now.


So, will you do it? :)

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/Notification.h:23: 
On 2015/08/06 07:35:30, sergei wrote:
> Here the same, <memory> is not included by <vector>, it's included in cpp file
> by another header before including of this header. 

Ok, that's even worse then - very non-obvious and error-prone. Will you fix it?

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/Default...
File src/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/Default...
src/DefaultFileSystem.cpp:79: std::shared_ptr<std::istream> data)
On 2015/08/05 21:28:04, Felix Dahlke wrote:
> Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
be
> in one line again.

Sergei didn't reply for some reason but this won't actually fit into 80 columns.

[Felix Dahlke, 2015-08-06 18:19:02]

Yup, just these two `#include <memory>`'s Wladimir suggested and this looks good
to land.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/FilterEngine.h:40: public
std::enable_shared_from_this<Filter>
On 2015/08/06 12:24:54, Wladimir Palant wrote:
> On 2015/08/05 21:28:04, Felix Dahlke wrote:
> > Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
> be
> > in one line again.
> 
> Sergei didn't address this nit and I think he is right - wrapping here isn't
> because of the line length limit. See
>
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...
> - "When deriving from more than one class, put each on its own line."

Oh, I wasn't aware of that, fair enough.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/Default...
File src/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/src/Default...
src/DefaultFileSystem.cpp:79: std::shared_ptr<std::istream> data)
On 2015/08/06 12:24:54, Wladimir Palant wrote:
> On 2015/08/05 21:28:04, Felix Dahlke wrote:
> > Nit: This was wrapped because it didn't fit in 80 columns. Now it would, can
> be
> > in one line again.
> 
> Sergei didn't reply for some reason but this won't actually fit into 80
columns.

True, my bad.

[sergei, 2015-08-07 06:29:40]

Sorry, I was quite inattentive on the previous patch set.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/JsValue.h:24: #include "V8ValueHolder.h"
On 2015/08/06 12:24:54, Wladimir Palant wrote:
> On 2015/08/06 07:35:30, sergei wrote:
> > <vector> does not include <memory>, it's included by "V8ValueHolder.h".
Anyway
> I
> > find it a good idea to include <memory> here now.
> 
> 
> So, will you do it? :)

Sorry, overlooked.

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
File include/AdblockPlus/Notification.h (right):

https://codereview.adblockplus.org/5163715573841920/diff/29323254/include/Adb...
include/AdblockPlus/Notification.h:23: 
On 2015/08/06 12:24:54, Wladimir Palant wrote:
> On 2015/08/06 07:35:30, sergei wrote:
> > Here the same, <memory> is not included by <vector>, it's included in cpp
file
> > by another header before including of this header. 
> 
> Ok, that's even worse then - very non-obvious and error-prone. Will you fix
it?

Yes, sure.

[Felix Dahlke, 2015-08-07 07:01:59]

LGTM!