24.11.2013 21:06, Max Kellermann пишет: > My few cents: Thanks for the comments!
> > - you reordered code in event/Loop.cxx, which makes reading the diff > more difficult than necessary That's my fault, probably I was addicted to Add/Modify/Remove trio in every class :-) > > - the PollGroup/PollGroupImpl indirection adds unnecessary runtime > overhead, without any advantage This is done to hide implementation details and use single header for all implementations, but if you prefer to avoid that I could remove the indirection. > > - why did you remove class EPollFD? Instead, PollGroupEPoll contains > this abstraction code, plus the PollGroupImpl code. This is done to encourage using universal PollGroup. > - why add overhead by storing the events in EventQueue? My > implementation could work without this storage class. Your commit > adds this overhead even to the epoll implementation. This adds some overhead for the purpose of having single API for all backends. Another option is to use backend specific way to deliver events (e.g. make each backend provide its own variant of EventQueue that could be passed to syscall directly - or even intergrate whole event queue into the PollGroup). Since this is a prototype I've started with simplest approach of using std::deque, but I agree this could be improved. Alternative approach is to use some callback mechanism, but I find pull-style API more convenient to use. > > - why implement the SocketSet copy constructor? This is done to avoid copying whole fd_array, but instead copy only actually used elements. > > - you removed all the GLib code in that one commit, and added all new > (untested) implementations; much of the commit deals with removing > GLib specific code > > The last one is unfortunate, because it means we have to do the full > switch in one big step. > > It would be better to have incremental patches which add one > (optional) new implementation at a time; now we have to be sure all of > them are mature at the same time. > > At the end, when we are portable across all platforms, we could remove > the GLib implementation. > I didn't expect this to be merged now. My plan was to polish the code until it is usable for development branch and then propose a merge. But probably we could make this more incremental and keep GLib code for a while. If so I think there should be a configure option to enable new event loop (or disable old). What do you think? I want to add one more backend that uses regular select(). In theory these 4 backends should cover most of the supported platforms. -- Denis ------------------------------------------------------------------------------ Shape the Mobile Experience: Free Subscription Software experts and developers: Be at the forefront of tech innovation. Intel(R) Software Adrenaline delivers strategic insight and game-changing conversations that shape the rapidly evolving mobile landscape. Sign up now. http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk _______________________________________________ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
