You know it's not a race guys... The reason that no-one has changed the code yet is that we haven't really had enough time to talk about it. The changes will get made, but it requires patience at the moment.
> > 1) Said before, but asserts are yucky. Once we have it compiling and > > working, this will be critical to remove. > > I guess this will stay a difference in opinion, even within the APR team. > But, since this code is (going to be) part of the APR library, it is now > your call. Personally I would have my code rather die at debug time so > I am confronted with my stupidity (and as a side-effect have faster running > release code, because the asserts are defined to noop) than have the code > silently continue and somewhere else fail. In the latter case I even > have a harder time finding where the problem is. This will be addressed... > > 2) From the performance standpoint, we need to make this malloc stuff as > > fast as the plain malloc call or this isn't going to be worth much. > > Checks such as "if !size return 0" probably isn't needed in > > apr_sms_malloc() code. > > Probably not. What does malloc() return when passed a size of 0? > Btw, sms _always_ adds some overhead, but what you get back for it is > (a bit of) flexibility. This will need looking at. Lets get some working code that we all agree is the way forward before we start getting nitpicky shall we? > > > 3) There is some logic that you are attempting to duplicate in > > apr_sms_realloc() that I believe the realloc system call handles for > you > > (at least on Solaris). More generally, you are *probably* better off > > letting the system calls handle this type of redirection rather than > > yourself, but I might be wrong. Dean may have a better idea > > whether we should try to outsmart the system calls or not - my gut > > feeling would be to avoid this "cleverness." > > The same goes for realloc() as goes for malloc(), however, this 'cleverness' > is minor overhead and lessens the work for a sms implementor. For > performance > I have to agree, you move the 'cleverness' to the specific sms > implementation, > or you don't implement it at all. IMHO the only sms that doesn't need the > 'cleverness' is the standard sms. Same as above. > > > (You also try to be "safe" about a memset call - if memset is bogus, > > we have lots of other problems...) > > Ok, so we ditch the memset and set all members of apr_sms_t to NULL by hand. Consider it yanked. > > 4) The function and type rename is a good step, but I'd probably like to > > see shorter local variable names. Makes for some long lines that have > > odd wrapping properties. This isn't my code, so it's not a big deal to > > me. Longer variables names are *slightly* better than completely > cryptic > > variable names... > > Like I stated before, the code was also the documentation in our discussion > (among Elrond, Luke and me). I have no problem with shorter variable names, > as long as the code stays readable. I tend to believe that for at least 25% > of the (to be) shortened variable names a comment is introduced. And if it's me that goes through and does the renaming this will be taken care of. > > 5) What do you plan apr_sms_threadsafe_lock to do (maybe you talked about > > this, but I missed it)? Is this something similar to apr_lock? Why are > > we concerned about an apr_sms_t being shared across threads? (Please > > don't tell me shared memory segments...) > > *grin* It was meant as the start of support for the thing I shouldn't tell > you. > The name is confusing as I stated in earlier postings. There has been much discussion of this already. > > 6) Somewhere there is a bunch of if, else if statements that are > > separated by a bunch of blank lines. Yuck. (apr_memory_system.c > around > > line 400.) > > I posted a 'typed cleanup function' patch that includes fixes for this (as a > side-effect of better status reporting). Well, there are lots of cleanups still required in the code. It's taken a fair bit of time to get it into the codebase as it stands now, and more effort will be required. When I get a better connection I'll be doing more. > > 7) Why is this stuff in memory/unix? Wouldn't most of this work > > on Win32, etc.? ANSI C requires most of the stuff you are using in > > apr_sms_std (malloc, free, etc.). But, I could be missing something. > > I believe David Reid already answered this one. I quote: > > "Try looking at the rest of the code in APR :) Win32 can build in a Unix > directory, but eventualy there will be some custom Win32 code and so we'll > have a memory_common.c in the win32 directory that simply includes the code > in the unix directory." Hopefully that answers it. > > > 8) Someone with karma should probably rename the files to match the > > appropriate names (apr_sms.h, etc.). > > Ack. Could someone with the karma please rise :-) There are a lot of people with "karma" but as I said the way we do things is to discuss then commit. AFAIK that's how things have always been done round here, so please try to be patient. It will happen. > > 9) Why is apr_tracking_memory_system.h separated out? This patch > > pulls it back in to (what should be) apr_sms.h. I can't figure out > > any logical reason why apr_tracking_memory_system.h exists. > > Because 'tracking sms' and 'standard sms' are just two implementations. > There are a lot more to come (I hope). Since you are always going to > need the standard sms it is included in (what should be) apr_sms.h. > The 'tracking sms' (only provided as an example btw) is just on of the > possible implementations of a custom tracking sms. It is therefor a > seperate module. Speaking of modules, someone already suggested dynamicly > loading sms modules :-), which could be another good reason. I would have thought this was an obvious step, but maybe not. Hopefully the explanation helped. > > To make another thing clear, I think it should be allowed to do platform > specific memory systems. On some platforms there is probably very fast > memory > allocation availabe and it should be possible to use that (but this is just > my opinion). Well we'll have to see where this all goes won't we? david
