Dave Miner wrote: > Evan Layton wrote: >> Ethan Quach wrote: >>> >>> Dave Miner wrote: >>>> Ethan Quach wrote: >>>> >>>>> Tim Knitter wrote: >>>>> >>>>>> Ethan, >>>>>> >>>>>> Nit: >>>>>> 586: Can we change newmp to be consistent with the other variables >>>>>> that handle mountpoints, e.g new_mntpt or something similar. >>>>>> >>>>>> 882: Can you explain in the comment why we try to re-create a BE >>>>>> when it is requested to be an auto-named BE? >>>>>> >>>>>> 988: successfuly... spelling >>>>>> >>>>>> 998: mounted -> mount >>>>>> >>>>>> 1120: Line not needed since free() handles NULL pointers. >>>>>> >>>>> We do this in a lot of places and the reason is that checking the >>>>> handle against NULL and not calling free() is cheaper then actually >>>>> calling free() with a NULL handle. >>>>> >>>>> >>>> In general, I agree with Tim (and make this comment a lot). If >>>> you're in a critical section, maybe; otherwise it's just noise. >>>> >>> >>> yeah, I think deep down I always agreed too :-) I'll change it here >>> and right >>> above at 1117. There are probably over 100 places we do this in >>> libbe though >>> so if you don't mind I'll file a separate bug to handle all those. >> >> I disagree that we need a bug for this. >> >> We've removed these in other sections based on comments from various >> people. However I still have trouble seeing this as a big deal or that >> it's just noise. It does eliminate a context switch for the free if >> we're passing in a NULL. I know that for me this is left over from >> kernel hacking but it still makes sense to me to avoid this context >> switch if we don't need to make the call to free. >> > > It litters the code with checks that constitute micro-optimizations, > without evidence that they're needed. IMHO if you're in state where > that's a measurable problem, mallocing and freeing memory is almost > certainly the wrong thing to be doing anyway.
I definitely have to agree with that last part! :-) The problem is that in many of these instances we're making calls into other libraries that do the malloc so we're kind of at their mercy with respect to malloc and free. As far as littering the code I'll just agree to disagree and we'll remove these checks anyway. ;-) -evan
