remove NotifdEvent #295
remove NotifdEvent #295
Conversation
Ingvord
force-pushed
the
refactoring-RemoveNotifd
branch
from
4a16bc8 to
8497f57
Compare
Ingvord
force-pushed
the
refactoring-RemoveNotifd
branch
from
347bb1c to
e4b2b60
Compare
bourtemb
left a comment
bourtemb
left a comment
There was a problem hiding this comment.
Sorry for reviewing only now this big PR.
Some conflicts need to be solved now.
I think it was a good idea to extract the EventConsumer stuff into new files.
I personally do not like the introduction of a new file extension (.cxx and .txx instead of .cpp and .tpp extension which is used on the other files).
Of course, this helps in seeing quickly what new files have been created in Tango 10 but I would rather go to something consistent among the project. And to simplify the maintenance (mainly when you need to look at the history of the files), I would rather keep the current .cpp and .tpp file extension.
src/server/eventcmds.cpp
Outdated
| else | ||
| client_release = 3; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this EventSubscriptionChange command is obsolete and is really notifd specific.
It could be kept for backwards compatibility, but should probably throw an exception.
There was a problem hiding this comment.
It looks like we could throw an exception when ct==NOTIFD in Dserver::event_subscription method too at the following line:
https://github.com/tango-controls/cppTango/pull/295/files#diff-08b411adab5ed25ab9d8495f4dd6d8e7L220
And some additional cleanup could be done on this method. (removing attribute.set_use_notifd_event())
| NotifdEventConsumer *notifd_event_consumer, | ||
| map<string,EventCallBackStruct>::iterator &epos, | ||
| map<string,EventChannelStruct>::iterator &ipos, | ||
| string &domain_name) |
There was a problem hiding this comment.
It looks like lines 1327 and 1328 could be removed too.
|
Well done on getting this big PR merged! |
3 participants
Progress: remove NotifdEventConsumer