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. Dave
