Dave Miner 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. >> > > Apparently code I didn't review ;-) Fine. > > Dave
actually you did make this comment in the original code review and we got rid of most of these then :-) However some did get missed :-( and it's a hard habit to brake. ;-) -evan
