Feature #2279
closed
Signal: allow only owner to emit events
Added by Alex Afanasyev over 9 years ago. Updated over 9 years ago.
100%
Description
template<typename Owner, typename ...TArgs>
class Signal
{
...
private:
void
operator()(const TArgs&... args) const;
friend Owner;
};
class Face
{
public:
Signal<Face, Interest> onReceiveInterest;
protected:
void
emit_onReceiveInterest(const Interest& interest)
{
onReceiveInterest(interest);
}
};
Updated by Alex Afanasyev over 9 years ago
- Related to Task #2116: Rewrite EventEmitter added
Updated by Junxiao Shi over 9 years ago
- Tracker changed from Task to Feature
- Subject changed from Proposal for protecting emission events by the EventEmitter to EventEmitter: allow only owner to emit events
- Description updated (diff)
- Category set to Utils
- Assignee set to Junxiao Shi
- Target version set to v0.3
I agree with this idea. Some details are changed to make it work.
But this is a backwards-incompatible change. I suggest:
- abandon the pending change of #2116
- keep
EventEmitteras is but mark it deprecated - create a
Signaltemplate that takesOwnertemplate argument - use Boost.Signals2 naming in
Signaltemplate Signal<TArgs...>::operator+=is not declared
Updated by Alex Afanasyev over 9 years ago
I agree with the proposed plan with the exception that we remove objective of event unsubscribing (mark them as strikethrough, not just delete. so it will be clear what all conversations are about) and declare the task closed.
Updated by Alex Afanasyev over 9 years ago
Btw. Anybody knows if boost.signal2 or other signal libraries provide protection for event emission? If so, how do they do that?
Updated by Junxiao Shi over 9 years ago
Boost.Signals2 doesn't have protection.
C# has protection, which is a built-in feature of Common Language Runtime.
Updated by Davide Pesavento over 9 years ago
Alex Afanasyev wrote:
Btw. Anybody knows if boost.signal2 or other signal libraries provide protection for event emission? If so, how do they do that?
In Qt signals are protected member functions, so this is not a problem for them.
Updated by Davide Pesavento over 9 years ago
There's one thing I don't like about the proposed solution: Owner has complete access to all EventEmitter internals, not just to signal emission.
Updated by Junxiao Shi over 9 years ago
- Subject changed from EventEmitter: allow only owner to emit events to Signal: allow only owner to emit events
- Description updated (diff)
- Status changed from New to In Progress
- Estimated time set to 3.00 h
Updated by Junxiao Shi over 9 years ago
- Description updated (diff)
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
Benchmark
// g++ x.cpp --std=c++0x -O3 `pkg-config --cflags --libs libndn-cxx`
#include <ndn-cxx/util/time.hpp>
#include <ndn-cxx/util/event-emitter.hpp>
#include <ndn-cxx/util/signal.hpp>
int g_sum;
void
f(int n)
{
g_sum += n;
}
struct EventEmitterOwner
{
ndn::util::EventEmitter<int> ee;
void
run()
{
for (int i = 0; i <= 99999999; ++i) {
ee(1);
}
}
};
struct SignalOwner
{
ndn::util::Signal<SignalOwner, int> sig;
void
run()
{
for (int i = 0; i <= 99999999; ++i) {
sig(1);
}
}
};
int
main()
{
g_sum = 0;
EventEmitterOwner eo;
eo.ee += &f;
auto t1 = ndn::time::system_clock::now();
eo.run();
auto t2 = ndn::time::system_clock::now();
std::cout << g_sum << " " << (t2 - t1) << std::endl;
g_sum = 0;
SignalOwner so;
so.sig.connect(&f);
auto t3 = ndn::time::system_clock::now();
so.run();
auto t4 = ndn::time::system_clock::now();
std::cout << g_sum << " " << (t4 - t3) << std::endl;
return 0;
}
Result for commit 6452abf877a50aa8503b7053b37331d782ad2b29:
| OS | EventEmitter | Signal |
|---|---|---|
| Ubuntu 12.04 | 6.806ns/op | 4.905ns/op |
| OSX 10.9 | 4.870ns/op | 4.549ns/op |
Updated by Junxiao Shi over 9 years ago
- Blocks Feature #2272: Strategy API: access to FaceTable added
Updated by Davide Pesavento over 9 years ago
Given the complexity and ugliness of the resulting implementation (especially the part to make signal emission from subclasses work), and given the fact that Signal internals are completely exposed to the Owner class, I'm not sure if this is worthwhile anymore, not in its current form at least (but I don't have an alternative solution).
Updated by Alex Afanasyev over 9 years ago
I had similar comment, but haven't mentioned it before... Also not sure if it worth the effort to do such complex thing that results in not so nice syntax... We could give a try for the latest version, but I'm having doubts that we will not refert that in the future.
Updated by Junxiao Shi over 9 years ago
- Status changed from Code review to Closed
Updated by Junxiao Shi about 9 years ago
- Blocks Feature #1529: PIB service added