Issue 5013 - Make more methods const.- introduced JsConstValuePtr and JsConstValueList- JsValue:…

Issue 5013 - Make more methods const.

- introduced JsConstValuePtr and JsConstValueList
- JsValue::Call() now take a JsConstValueList or JsConstValuePtr
- Use the single JsConstValuePtr overload of Call() when appropriate.

[sergei, 2017-03-23 19:46:07]

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:45: typedef std::shared_ptr<const JsValue>
JsConstValuePtr;
What about ConstJsValuePtr?

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:51: typedef
std::vector<AdblockPlus::JsConstValuePtr> JsConstValueList;
What about ConstJsValueList?

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:126: JsValuePtr Call(const JsConstValueList&
params = JsConstValueList(),
This change is a bit questionable, but I think we can let it be for present.

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
There is actually `JsValue::Call(const JsValue& arg)` what about changing in the
same commit the call of the function, `func->Call(*this)->AsBool();`?

It's also valid for many cases below.

[hub, 2017-03-24 13:45:15]

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:51: typedef
std::vector<AdblockPlus::JsConstValuePtr> JsConstValueList;
On 2017/03/23 19:46:06, sergei wrote:
> What about ConstJsValueList?

To answer about here and above, I feel like ConstJsValueList here seems to be
ambiguous by reading the type name. Is it the List that is const or the JsValue.

My thinking for naming the type was that "Js" is a namespace prefix. So it felt
like JsConstValuePtr was appropriate, and then JsConstValueList was obvious as
being a list of JsConstValuePtr.

We don't have other types using const yet, so this somewhat set the precedent.
So, it is as you prefer.

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:126: JsValuePtr Call(const JsConstValueList&
params = JsConstValueList(),
On 2017/03/23 19:46:06, sergei wrote:
> This change is a bit questionable, but I think we can let it be for present.

it is quite essential. The problem boils down to the Getter() function passing a
pointer to this (shared_from_this) as a parameter to Call() which make it
impossible to make the function const.
Note that line 135, the overloaded version just gets a const JsValue& as a
single arugment, so it is actually consistent here.

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
On 2017/03/23 19:46:06, sergei wrote:
> There is actually `JsValue::Call(const JsValue& arg)` what about changing in
the
> same commit the call of the function, `func->Call(*this)->AsBool();`?
> 
> It's also valid for many cases below.

Now that I'm looking at it, I think that this function should even be changed to
take a const JsConstValuePtr& so that we don't have to unwrap and reallocate a
new JsValue. And replacing all these calls where params has only one element.

[sergei, 2017-03-24 13:56:47]

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:51: typedef
std::vector<AdblockPlus::JsConstValuePtr> JsConstValueList;
On 2017/03/24 13:45:15, hub wrote:
> On 2017/03/23 19:46:06, sergei wrote:
> > What about ConstJsValueList?
> 
> To answer about here and above, I feel like ConstJsValueList here seems to be
> ambiguous by reading the type name. Is it the List that is const or the
JsValue.
> 
> My thinking for naming the type was that "Js" is a namespace prefix. So it
felt
> like JsConstValuePtr was appropriate, and then JsConstValueList was obvious as
> being a list of JsConstValuePtr.
> 
> We don't have other types using const yet, so this somewhat set the precedent.
> So, it is as you prefer.

Agree. Considering Js as a namespace makes sense here.

https://codereview.adblockplus.org/29393589/diff/29393590/include/AdblockPlus...
include/AdblockPlus/JsValue.h:126: JsValuePtr Call(const JsConstValueList&
params = JsConstValueList(),
On 2017/03/24 13:45:15, hub wrote:
> On 2017/03/23 19:46:06, sergei wrote:
> > This change is a bit questionable, but I think we can let it be for present.
> 
> it is quite essential. The problem boils down to the Getter() function passing
a
> pointer to this (shared_from_this) as a parameter to Call() which make it
> impossible to make the function const.
> Note that line 135, the overloaded version just gets a const JsValue& as a
> single arugment, so it is actually consistent here.

I rather thought about no replacing the existing method but adding yet one for
JsConstValueList but maybe it's even better to have only const
JsConstValueList&.
So, it's OK.

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
On 2017/03/24 13:45:15, hub wrote:
> On 2017/03/23 19:46:06, sergei wrote:
> > There is actually `JsValue::Call(const JsValue& arg)` what about changing in
> the
> > same commit the call of the function, `func->Call(*this)->AsBool();`?
> > 
> > It's also valid for many cases below.
> 
> Now that I'm looking at it, I think that this function should even be changed
to
> take a const JsConstValuePtr& so that we don't have to unwrap and reallocate a
> new JsValue. And replacing all these calls where params has only one element.

We don't need to reallocate new JsValue here.

Actually I would like to reduce the number of cases when we use
std::shared_ptr<JsValue> and rather just use JsValue. So, we could add a helper
Call method accepting `const JsConstValuePtr&` but I would like to try without
it and actually see whether we need it.

[hub, 2017-03-24 14:08:03]

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
On 2017/03/24 13:56:47, sergei wrote:
> On 2017/03/24 13:45:15, hub wrote:
> > On 2017/03/23 19:46:06, sergei wrote:
> > > There is actually `JsValue::Call(const JsValue& arg)` what about changing
in
> > the
> > > same commit the call of the function, `func->Call(*this)->AsBool();`?
> > > 
> > > It's also valid for many cases below.
> > 
> > Now that I'm looking at it, I think that this function should even be
changed
> to
> > take a const JsConstValuePtr& so that we don't have to unwrap and reallocate
a
> > new JsValue. And replacing all these calls where params has only one
element.
> 
> We don't need to reallocate new JsValue here.
> 
> Actually I would like to reduce the number of cases when we use
> std::shared_ptr<JsValue> and rather just use JsValue. So, we could add a
helper
> Call method accepting `const JsConstValuePtr&` but I would like to try without
> it and actually see whether we need it.

In JsValue::Call() we currently do:

params.push_back(JsValuePtr(new JsValue(arg.jsEngine, arg.UnwrapValue())));

Basically UnwrapValue() from const to a new value that we insert.

If we pass a JsConstValuePtr, we no longer need to do that.

[sergei, 2017-03-24 14:24:55]

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
On 2017/03/24 14:08:03, hub wrote:
> On 2017/03/24 13:56:47, sergei wrote:
> > On 2017/03/24 13:45:15, hub wrote:
> > > On 2017/03/23 19:46:06, sergei wrote:
> > > > There is actually `JsValue::Call(const JsValue& arg)` what about
changing
> in
> > > the
> > > > same commit the call of the function, `func->Call(*this)->AsBool();`?
> > > > 
> > > > It's also valid for many cases below.
> > > 
> > > Now that I'm looking at it, I think that this function should even be
> changed
> > to
> > > take a const JsConstValuePtr& so that we don't have to unwrap and
reallocate
> a
> > > new JsValue. And replacing all these calls where params has only one
> element.
> > 
> > We don't need to reallocate new JsValue here.
> > 
> > Actually I would like to reduce the number of cases when we use
> > std::shared_ptr<JsValue> and rather just use JsValue. So, we could add a
> helper
> > Call method accepting `const JsConstValuePtr&` but I would like to try
without
> > it and actually see whether we need it.
> 
> In JsValue::Call() we currently do:
> 
> params.push_back(JsValuePtr(new JsValue(arg.jsEngine, arg.UnwrapValue())));
> 
> Basically UnwrapValue() from const to a new value that we insert.
> 
> If we pass a JsConstValuePtr, we no longer need to do that.

I think that we should simply change the implementation of JsValue::Call(const
JsValue&) to don't create new JsValue and JsValuePtr. Finally I would like to
get rid of JsValuePtr because on practice there is no need for it.

[hub, 2017-03-24 16:05:06]

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
On 2017/03/24 14:24:55, sergei wrote:

> I think that we should simply change the implementation of JsValue::Call(const
> JsValue&) to don't create new JsValue and JsValuePtr. Finally I would like to
> get rid of JsValuePtr because on practice there is no need for it.

Agreed. Issue 3589?

[sergei, 2017-03-24 16:27:52]

LGTM

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29393589/diff/29393590/src/FilterEngine.cp...
src/FilterEngine.cpp:62: return func->Call(params)->AsBool();
On 2017/03/24 16:05:06, hub wrote:
> On 2017/03/24 14:24:55, sergei wrote:
> 
> > I think that we should simply change the implementation of
JsValue::Call(const
> > JsValue&) to don't create new JsValue and JsValuePtr. Finally I would like
to
> > get rid of JsValuePtr because on practice there is no need for it.
> 
> Agreed. Issue 3589?

I think it should be another issue (https://issues.adblockplus.org/ticket/5034)
and #3589 should be a part of it. So basically changing to `func->Call(*this)`
should be done as #5034.