Issue 5013 - Make parameter const ref when applicable.

Issue 5013 - Make parameter const ref when applicable.

[hub, 2017-04-11 15:46:59]

Updated patch following our discussion

[sergei, 2017-04-12 13:34:31]

https://codereview.adblockplus.org/29409580/diff/29409581/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29409580/diff/29409581/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:297: void CallTimerTask(const
TimerTasks::const_iterator& timerTaskIterator);
I'm not sure that passing of const_iterator of std::list by reference makes
sense.

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp#newco...
src/Utils.cpp:30: std::string Utils::Slurp(const std::istream& stream)
I would like to avoid having const here because we actually modify the stream
object, we get a non-const pointer to basic_stringbuf and modify it.

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp#newco...
src/Utils.cpp:37: std::string Utils::FromV8String(const v8::Handle<v8::Value>&
value)
I'm not sure that v8::Handle should be passed by reference because its copy
constructor is merely copying internally held pointer.
It's also in JsError.{h,cpp}.

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
include/AdblockPlus/DefaultFileSystem.h:41: void Write(const std::string& path,
const std::istream& data);
I'm not sure that it should be const because std::istream is modified when one
reads from it. Actually, using of a stream here looks redundant, as I see we
convert std::string to a stream and then read a string from the stream. We could
simply use original string, but it's another issue.

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
File src/AppInfoJsObject.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
src/AppInfoJsObject.cpp:26: JsValuePtr AppInfoJsObject::Setup(const AppInfo&
appInfo, const JsValuePtr& obj)
Here and in other *JsObject files it seems it would be better to simply pass
`JsValue&`.

https://codereview.adblockplus.org/29409580/diff/29410555/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/Utils.h#newcode32
src/Utils.h:32: std::string Slurp(const std::istream& stream);
The same
https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...

[hub, 2017-04-12 13:51:35]

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
include/AdblockPlus/DefaultFileSystem.h:41: void Write(const std::string& path,
const std::istream& data);
On 2017/04/12 13:34:31, sergei wrote:
> I'm not sure that it should be const because std::istream is modified when one
> reads from it. Actually, using of a stream here looks redundant, as I see we
> convert std::string to a stream and then read a string from the stream. We
could
> simply use original string, but it's another issue.

Current implementation calls std::basic_ios::rdbuf() on it which takes a const.
But we shouldn't assume that and operator>> doesn't take a const instance. I'll
change this.

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
File src/AppInfoJsObject.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
src/AppInfoJsObject.cpp:26: JsValuePtr AppInfoJsObject::Setup(const AppInfo&
appInfo, const JsValuePtr& obj)
On 2017/04/12 13:34:31, sergei wrote:
> Here and in other *JsObject files it seems it would be better to simply pass
> `JsValue&`.

I have that in another patch. But I can put it in this one of you want.

https://codereview.adblockplus.org/29409580/diff/29410555/src/Utils.h
File src/Utils.h (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/Utils.h#newcode32
src/Utils.h:32: std::string Slurp(const std::istream& stream);
On 2017/04/12 13:34:31, sergei wrote:
> The same
>
https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...

It calls istream::rdbuf() which takes a const object.

http://en.cppreference.com/w/cpp/io/basic_ios/rdbuf

[sergei, 2017-04-12 13:57:25]

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
include/AdblockPlus/DefaultFileSystem.h:41: void Write(const std::string& path,
const std::istream& data);
On 2017/04/12 13:51:35, hub wrote:
> On 2017/04/12 13:34:31, sergei wrote:
> > I'm not sure that it should be const because std::istream is modified when
one
> > reads from it. Actually, using of a stream here looks redundant, as I see we
> > convert std::string to a stream and then read a string from the stream. We
> could
> > simply use original string, but it's another issue.
> 
> Current implementation calls std::basic_ios::rdbuf() on it which takes a
const.
> But we shouldn't assume that and operator>> doesn't take a const instance.
I'll
> change this.

That's the issue, the method is const but it returns a pointer to non-const
object but it's again this thing which marked as bold in the issue description.

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
File src/AppInfoJsObject.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
src/AppInfoJsObject.cpp:26: JsValuePtr AppInfoJsObject::Setup(const AppInfo&
appInfo, const JsValuePtr& obj)
On 2017/04/12 13:51:35, hub wrote:
> On 2017/04/12 13:34:31, sergei wrote:
> > Here and in other *JsObject files it seems it would be better to simply pass
> > `JsValue&`.
> 
> I have that in another patch. But I can put it in this one of you want.

Will it be possible to firstly commit that another patch and then the current
one?

[hub, 2017-04-12 14:04:53]

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
File src/AppInfoJsObject.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
src/AppInfoJsObject.cpp:26: JsValuePtr AppInfoJsObject::Setup(const AppInfo&
appInfo, const JsValuePtr& obj)
On 2017/04/12 13:57:24, sergei wrote:
> On 2017/04/12 13:51:35, hub wrote:
> > On 2017/04/12 13:34:31, sergei wrote:
> > > Here and in other *JsObject files it seems it would be better to simply
pass
> > > `JsValue&`.
> > 
> > I have that in another patch. But I can put it in this one of you want.
> 

Actually this need a change to SetProperty to accept a const JsValue& as it
currently want a JsValuePtr. My other patch take care of this, but it is not
simple.

> Will it be possible to firstly commit that another patch and then the current
> one?

I don't think. I could try though.

[hub, 2017-04-12 14:42:27]

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
File include/AdblockPlus/DefaultFileSystem.h (right):

https://codereview.adblockplus.org/29409580/diff/29410555/include/AdblockPlus...
include/AdblockPlus/DefaultFileSystem.h:40: std::shared_ptr<std::istream>
Read(const std::string& path) const;
Also I have another patch that use unique_ptr<> here because we don't need it
shared.... (if the client needs it shared you can promote). I could roll that in
here. Otherwise it can wait.

[hub, 2017-04-12 15:06:14]

https://codereview.adblockplus.org/29409580/diff/29409581/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29409580/diff/29409581/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:297: void CallTimerTask(const
TimerTasks::const_iterator& timerTaskIterator);
On 2017/04/12 13:34:30, sergei wrote:
> I'm not sure that passing of const_iterator of std::list by reference makes
> sense.

still more than a plain copy since it is not a PoD.

[hub, 2017-04-12 15:17:03]

updated the patch

[hub, 2017-04-13 08:13:45]

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
File src/AppInfoJsObject.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29410555/src/AppInfoJsObject...
src/AppInfoJsObject.cpp:26: JsValuePtr AppInfoJsObject::Setup(const AppInfo&
appInfo, const JsValuePtr& obj)

> > Will it be possible to firstly commit that another patch and then the
current
> > one?
> 
> I don't think. I could try though.

Patch in question to apply after this is up for review.
https://codereview.adblockplus.org/29410664/

[sergei, 2017-04-18 14:02:15]

I have concerns only regarding references to v8::Handle. The rest is fine.

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp#newco...
src/Utils.cpp:37: std::string Utils::FromV8String(const v8::Handle<v8::Value>&
value)
On 2017/04/12 13:34:30, sergei wrote:
> I'm not sure that v8::Handle should be passed by reference because its copy
> constructor is merely copying internally held pointer.
> It's also in JsError.{h,cpp}.

What about it?

[hub, 2017-04-18 16:18:51]

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29409580/diff/29409581/src/Utils.cpp#newco...
src/Utils.cpp:37: std::string Utils::FromV8String(const v8::Handle<v8::Value>&
value)
On 2017/04/18 14:02:14, sergei wrote:
> On 2017/04/12 13:34:30, sergei wrote:
> > I'm not sure that v8::Handle should be passed by reference because its copy
> > constructor is merely copying internally held pointer.
> > It's also in JsError.{h,cpp}.
> 
> What about it?

I thought I had replied to this one. My take is that I just avoid the copy
constructor. I'm not supposed to know what the copy constructor does (ie barely
nothing) and passing the argument as const & just make sure we do it the best
(fastest) way.

[sergei, 2017-04-19 10:18:47]

LGTM