On 2015-09-19 23:57, Michael McConville wrote: >> What's your thinking behind this? To me this seems like a perfectly >> rational and well motivated function to have, both for readability and >> rather than having to repeat the same statements several times over in >> the rest of the code, risking future calamity if a change is made in >> one place and not the other... >> >> I mean, I don't know if I'm unusual, but I often find myself breaking >> out separate functions from larger pieces of code, that may end up >> being called just that one time in a program, just to improve >> readability and keep things logically apart. > > The most important thing to note is that this diff removes seven lines. > If you're abstracting something into a function, it definitely shouldn't > be creating more code. > > When you create functions for things like if (z) errx(...); conditions, > it makes the code much harder to read. Someone trying to figure out > what's going on has to find the function definition and remember what it > does. Extrapolate this for every two-line error condition, and you have > a major headache. Such functions improve readability only if you're the > one writing them. > > So, I wouldn't like this function no matter how many times it was used. > And it only appears twice.
I respectfully disagree. :-) Not only do I feel this function actually makes the code *easier* to read, even though one has to fish around for the function elsewhere first, it also makes maintenance more coherent and safer, in part due to what I mentioned above about for example accidentally changing say all but one occurances of the unrolled code... I also don't think one needs to be afraid to create more code in the name of clarity, at least not as in this case when it is a matter of only a few bytes that will likely be swallowed up by page size alignment anyway. And although the savings in execution time from inlining the function is always a factor to take into account I have a feeling it is negligible in this case. Further, if code readability is an argument, something that would have a much, much bigger impact than this diff would be to take a look at and try to do something about the horrible #ifdef madness in this piece of code. I mean, just look at some of the function declarations! They're making my eyes hurt... Finally, of course I am aware I don't really have a say in the matter. :-) I am just a little afraid of diffs that seem more for the sake of change than for any particular benefit, but as long as there is a sound thought behind I'll gladly shut up! And in any case, this is just the opinion of someone who has read and written unix code since before the Napoleonic wars (and is the proud owner of not one but two first edition K&R:s, printed in 1978), so my opinions on style are likely as old and antiquated as I am (or at least feel some mornings). Regards, /Benny
