a_Bw_stop_clients() is always called with the BW_Root and BW_Img flags set, so we can remove those flags. That leaves only the BW_Force flag, so we can remove that too and change the flags argument to a boolean that indicates whether to force. Regards, Jeremy Henty
I wrote:
a_Bw_stop_clients() is always called with the BW_Root and BW_Img flags set, so we can remove those flags. That leaves only the BW_Force flag, so we can remove that too and change the flags argument to a boolean that indicates whether to force.
What's the verdict on this? I haven't seen any comments. I have some other cleanups and refactoring to propose and some of those patches depend on this one, so it would be good know whether or not it will go in so I can ready the rest for submission. Regards, Jeremy Henty
On Thu, Mar 12, 2009 at 10:36:08AM +0000, Jeremy Henty wrote:
I wrote:
a_Bw_stop_clients() is always called with the BW_Root and BW_Img flags set, so we can remove those flags. That leaves only the BW_Force flag, so we can remove that too and change the flags argument to a boolean that indicates whether to force.
What's the verdict on this? I haven't seen any comments. I have some other cleanups and refactoring to propose and some of those patches depend on this one, so it would be good know whether or not it will go in so I can ready the rest for submission.
Sorry, I can't comment on this one as I don't know the relevant code enough. Let's wait for Jorge to comment. Cheers, Johannes
On Thu, Mar 12, 2009 at 08:38:21PM +0100, Hofmann Johannes wrote:
On Thu, Mar 12, 2009 at 10:36:08AM +0000, Jeremy Henty wrote:
I wrote:
a_Bw_stop_clients() is always called with the BW_Root and BW_Img flags set, so we can remove those flags. That leaves only the BW_Force flag, so we can remove that too and change the flags argument to a boolean that indicates whether to force.
What's the verdict on this? I haven't seen any comments. I have some other cleanups and refactoring to propose and some of those patches depend on this one, so it would be good know whether or not it will go in so I can ready the rest for submission.
Sorry, I can't comment on this one as I don't know the relevant code enough. Let's wait for Jorge to comment.
Sorry, I've had little time latelely... Anyway I reviewed the patch back then, and remembered that the current interface was meant for flexibility to experiment. The smantics for STOP were/are not well defined yet, and so the flags API lingered. I have to give it a second review. Anyway, if there're more cleanups pending on this patch, it may be worth accepting them and at worst to re-introduce the flags when STOP semantics get high priority (if ever). -- Cheers Jorge.-
On Fri, Mar 13, 2009 at 08:40:45AM -0300, Jorge Arellano Cid wrote:
Anyway I reviewed the patch back then, and remembered that the current interface was meant for flexibility to experiment.
Yes, there is a case for keeping the flags for future development. I just wanted to point that they are currently redundant and we could remove them if we wanted.
I have to give it a second review. Anyway, if there're more cleanups pending on this patch, it may be worth accepting them
Just so you know, the cleanups I have pending refactor a lot of code that calls the dList_* API in convoluted and inefficient ways. (Most stuff that call dList_remove() are like this.) You get cleaner and more efficient code by defining new dList_remove_* and dList_free_* functions, because that lets you replace lots of convoluted code paths that call dList_remove(), dList_nth_data() etc. with single API calls. One function that gets refactored is a_Bw_stop_clients(), which is why submitting the cleanup patches is waiting on the decision about the BM_Flags patch. It won't be hard to fix the cleanup patches if the BM_Flags patch does not go in, but until the decision is made I don't know whether or not to do that. Regards, Jeremy Henty
Hi, On Fri, Mar 13, 2009 at 05:52:07PM +0000, Jeremy Henty wrote:
On Fri, Mar 13, 2009 at 08:40:45AM -0300, Jorge Arellano Cid wrote:
Anyway I reviewed the patch back then, and remembered that the current interface was meant for flexibility to experiment.
Yes, there is a case for keeping the flags for future development. I just wanted to point that they are currently redundant and we could remove them if we wanted.
I'd prefer to keep the flags.
I have to give it a second review. Anyway, if there're more cleanups pending on this patch, it may be worth accepting them
Just so you know, the cleanups I have pending refactor a lot of code that calls the dList_* API in convoluted and inefficient ways. (Most stuff that call dList_remove() are like this.) You get cleaner and more efficient code by defining new dList_remove_* and dList_free_* functions, because that lets you replace lots of convoluted code paths that call dList_remove(), dList_nth_data() etc. with single API calls.
dList API was designed with a minimal working function set in mind. This keeps the API easy to remmeber and use. Now, if there's a measurable performance gain with the added functions it may be worth adding them.
One function that gets refactored is a_Bw_stop_clients(), which is why submitting the cleanup patches is waiting on the decision about the BM_Flags patch. It won't be hard to fix the cleanup patches if the BM_Flags patch does not go in, but until the decision is made I don't know whether or not to do that.
Please try to get some performance numbers, and in the meanwhile send me a summary of the API changes. -- Cheers Jorge.-
On Wed, Mar 18, 2009 at 09:26:14AM -0400, Jorge Arellano Cid wrote:
On Fri, Mar 13, 2009 at 05:52:07PM +0000, Jeremy Henty wrote:
Yes, there is a case for keeping the flags for future development.
I'd prefer to keep the flags.
OK, I'll drop the patch and work on the rest.
Just so you know, the cleanups I have pending refactor a lot of code that calls the dList_* API in convoluted and inefficient ways. [...] [...] dList API was designed with a minimal working function set in mind. This keeps the API easy to remmeber and use.
I'm sure that's right but there's a lot of repeated code wherever dLists are cleaned up. That makes me think that the current API is a little *too* minimal.
Please try to get some performance numbers, and in the meanwhile send me a summary of the API changes.
I will. Regards, Jeremy Henty
participants (3)
-
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org