Opened 5 years ago

Closed 2 years ago

#1378 closed change (worksforme)

Pass event callbacks by reference

Reported by: fhd Assignee:
Priority: P3 Milestone:
Module: Libadblockplus Keywords:
Cc: trev, sergz Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by fhd)

Background

We're currently passing std::function objects by value in JsEngine::SetEventCallback and FilterEngine::SetFilterChangeCallback. Because of this, using stateful functors is not possible.

What to change

Pass event callbacks by reference in JsEngine::SetEventCallback, FilterEngine::SetFilterChangeCallback and FilterEngine::SetUpdateAvailableCallback. Event callbacks are currently being stored by value in an std::map in JsEngine - we'll have to store a pointer instead.

Once this is done, we should check client and test code that works with event callbacks - we will be able to get rid of some hacks there.

Change History (5)

comment:1 Changed 5 years ago by fhd

  • Cc trev sergz added

Adding Sergei and Wladimir.

What do you guys think? IMO this would be more intuitive and technically possible.

comment:2 Changed 5 years ago by fhd

  • Description modified (diff)

comment:3 Changed 5 years ago by sergz

I'm sharing by subjective opinion.

I would say that while we operate with std::function the statefulness should be implemented on the client side and the client should be ready that the delegate can be copied by the library several times. Such restriction is common in STL and it's more error-tolerant (one may make a mistake but it still does work).

But passing by the const ref would be definitely a good optimization, so I would change only this part.

If it somehow does not fit the client circumstances (for example, it may be difficult to store the pointer in closure and properly release it, who knows) I would replace std::function by interface with a single method, in this case we will not make a copy of instance by accident and the client is aware about that by API. Also the lifetime of the passed object should be clear for the client

  • FilterChangeCallback*/reference_wrapper<FilterChangeCallback> - not managed by the library, should be disposed by the client after destroying the filter instance
  • unique_ptr/auto_ptr<FilterChangeCallback> - the library is responsible for the disposing of the delegate
  • shared_ptr<FilterChangeCallback> - the delegate is available for the client as well as for the library and its lifetimes depends on the usage counter

comment:4 Changed 4 years ago by fhd

  • Description modified (diff)

comment:5 Changed 2 years ago by sergz

  • Resolution set to worksforme
  • Status changed from new to closed
  • Tester set to Unknown

We are passing callbacks by const ref everywhere and the client should be ready that the function object will be copied. Therefore, I'm closing this issue.

Note: See TracTickets for help on using tickets.