Re: removing useless if-before-free tests

[Jim Meyering]

Bruno Haible <address@hidden> wrote:
> Jim Meyering wrote:
>> I agree those should go, along with all of the others in tests/*.c.
>
> I agree about the tests. Tests can afford small slowdowns.
>
>> I'd like to avoid spending time looking for ways to micro-optimize.
>> How about if I remove them all, and then restore-with-justification
>> the ones for which you think there is a significant performance gain?
>> Then, at least, there will be some indication to future reviewers
>> why we've kept a seemingly redundant test.
>
> No, I heavily disagree with such an approach. It should be the other way
> around: Remove the 'if' only with a written justification to which the
> author agrees.

???  No one is proposing to modify your files without permission.

> - Saying "I will break your performance, because I don't have much time,
>   then _you_ please fix up what I broke" is simply unfriendly.

Yes, saying that would be unfriendly.
However, I did not say or imply any such thing, and wonder why
you try to attribute such unpleasantness to me.

A difference of a few instructions "breaks performance"???
Isn't that perhaps just a tiny exaggeration?

>   I don't have much more time than you have.

You have misread.  I offered to make both changes.
However, since I don't believe in making redundancy-adding changes
without evidence, and don't want to search for micro-optimizations,
I proposed to document whatever redundant tests you preferred to
retain, by re-adding them.  I am prepared to wait until you have
found time to justify whatever you want.  Of course, if you want
to make the changes yourself, that's even better.

> - Code that Paul, I, or Eric wrote has a lot of care and thought in it.
>   It is wrong to assume that because there is no comment at a certain place,
>   the author did not think about it.

You're reading way too much into my words.
There was no such assumption on my part.
However, I *do* assume that few people write

  if (p)
    free (p);

to save a few instructions over a bare "free (p);", when "p" is often
NULL.  Here, we are all well aware of gnulib's "free" module, so in code
that does not depend on that module, I think it's only fair to assume that
the redundant "if" is to avoid the risk of a nonconforming free function.

Personally, I would not add an extra test like that unless I'd
profiled the code in question and found a significant improvement.
And if I'd taken the time to profile and add the test, I would have
felt obliged to add a comment justifying code what otherwise might seem
redundant.

> - (A less relevant argument:) Among the occurrences of the pattern in lib/*.c
>   that I briefly looked at, the majority appears to be in the case where NULL
>   is frequently used, i.e. where the 'if' should be kept for performance.

I'm surprised that you would come to such a conclusion.
I've been taught that "preoptimization is the root of all evil" (Knuth).
If you've already made some performance measurements to support that
claim, please let us know.

> A different approach would be if you said: "I found some useless 'if's in
> these files - Bruno -, in these files - Eric -, in these files - Simon -,
> etc. Please look which of them you can remove." This way, you're friendly
> towards everyone, you don't break code that is not yours, and you don't
> need to spend much time.

I find it hard to understand your arguing for a more friendly attitude
when you yourself exhibit such a confrontational and abrasive manner.
-------------------

Try to see this from my perspective:
I saw this as a simple, mechanical, obviously-correct, and
non-controversial change.  I wanted to make the global change
efficiently (spending little of my time), and didn't anticipate
any objection.  That was my mind-set when I started this thread.

Imagine my surprise when you object, citing the potential for
this to be a "performance-breaking" change, and then consider
your abrasive manner and misrepresentations in the message to
which I am now replying.  I have to confess I'm taken aback.

You've made many fine contributions on the technical front.
Please don't spoil the atmosphere here.

Jim