Opened 4 years ago

Closed 3 years ago

#3589 closed change (fixed)

Change return type of AdblockPlus::JsValue::Call from pointer to r-value object.

Reported by: sergz Assignee:
Priority: P4 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, eric@…, oleksandr, rjeschke Blocked By:
Blocking: #5034 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29334678/

Description

Background

Since it has been commented here https://codereview.adblockplus.org/5598762307158016/diff/29334349/src/FilterEngine.cpp#newcode230 I would like to discuss in this issue the possibility of this change and if so, the scale of it.

  • It looks logical and less error prone to return a valid r-value from the method which represents a function call. The evidence of it is the current usage of it, we are passing it immediately somewhere using move semantics or calling some method on it or just ignoring it.
  • It is a significant conceptual change in the interface, currently we are extensively using JsValuePtr (std::shared_ptr<JsValue>), thus this change brings inconsistency. Should we also return r-value JsValue from JsValue::GetProperty, use it for JsValueList and add a copy constructor (it copies only the reference, not deep copy)?

If we continue to use std::shared_ptr we can face with some auxiliary transformations, e.g. to pass some JsValue (based) object as a parameter to some function (put it into params arg of JsValue::Call) we will need to create a copy of it stored in std::shared_ptr.

What to change

Change return type of AdblockPlus::JsValue::Call from JsValuePtr to JsValue.

The proposed change

One can find in review https://codereview.adblockplus.org/29334678/ how it could be.

Change History (3)

comment:1 Changed 3 years ago by sergz

  • Blocking 5034 added

comment:3 Changed 3 years ago by sergz

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.