Thread: Re: [COMMITTERS] pgsql: Remove dead assignment

Peter Eisentraut <peter_e@gmx.net> writes:
> Remove dead assignment
> found by Coverity
   init_sequence(seq_relid, &elm, &seq_rel);
-   seq = read_info(elm, seq_rel, &buf);
+   read_info(elm, seq_rel, &buf);


I have to object to this patch.  In the blind service of eliminating
warnings from some tool or other, you will introduce warnings from
other tools?  It's traditional for lint to complain about code that
sometimes ignores the return value of a function, for instance.
I also do not think it does anything for readability for this call
of read_info() to be unexpectedly unlike all the others.

I think we should institute a project policy that we will ignore "dead
assignment" coverity warnings.  I have not seen one of those changes
yet that seemed to me like a good idea.  Any optimizing compiler is
perfectly capable of figuring out that an assignment is dead and
eliminating it, so there is no code size advantage from doing this
manually; and even the gcc boys have not (yet?) decided they should
warn about dead assignments.
        regards, tom lane

On mån, 2012-03-26 at 15:15 -0400, Tom Lane wrote:
>     init_sequence(seq_relid, &elm, &seq_rel);
> -   seq = read_info(elm, seq_rel, &buf);
> +   read_info(elm, seq_rel, &buf);
> 
> 
> I have to object to this patch.  In the blind service of eliminating
> warnings from some tool or other, you will introduce warnings from
> other tools?  It's traditional for lint to complain about code that
> sometimes ignores the return value of a function, for instance.

Yes, but the return value is ignored in this case as well.  Just
assigning it doesn't change that.

> I also do not think it does anything for readability for this call
> of read_info() to be unexpectedly unlike all the others. 

I do not think that it is good code quality to assign something to a
variable and then assign something different to a variable later in the
same function.  It is better, on the other hand, if a function call
looks different if what it's supposed to do is different.

But I don't want to get hung up on this.  I thought it was just an
oversight.

Peter Eisentraut <peter_e@gmx.net> writes:
> On mån, 2012-03-26 at 15:15 -0400, Tom Lane wrote:
>> I also do not think it does anything for readability for this call
>> of read_info() to be unexpectedly unlike all the others. 

> I do not think that it is good code quality to assign something to a
> variable and then assign something different to a variable later in the
> same function.

Well, that's a totally different issue, because if we had used a
different variable for the other purpose, this assignment would
still be dead and coverity would still be whinging about it, no?

The problem that I have with this change (and the similar ones you've
made elsewhere) is really that it's only chance that the code isn't
fetching anything from the result of read_info.  If we subsequently
wanted to change the logic so it did do that, we'd have to put back the
assignment.  That sort of code churn benefits nobody.
        regards, tom lane

On mån, 2012-03-26 at 15:53 -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> > On mån, 2012-03-26 at 15:15 -0400, Tom Lane wrote:
> >> I also do not think it does anything for readability for this call
> >> of read_info() to be unexpectedly unlike all the others. 
> 
> > I do not think that it is good code quality to assign something to a
> > variable and then assign something different to a variable later in the
> > same function.
> 
> Well, that's a totally different issue, because if we had used a
> different variable for the other purpose, this assignment would
> still be dead and coverity would still be whinging about it, no?

Let's look at this differently.  If this code were written from scratch
now, it might have turned out like this:

Form_pg_sequence old_seq, seq;

...
old_seq = read_info(elm, seq_rel, &buf);
...
seq = (Form_pg_sequence) GETSTRUCT(tuple);

But that gets a complaint from gcc:

sequence.c:248:19: error: variable ‘old_seq’ set but not used [-Werror=unused-but-set-variable]

So when faced with this, what is the right fix?  (Probably not assigning
the useless return value to some other variable used for a different
purpose.)

> The problem that I have with this change (and the similar ones you've
> made elsewhere) is really that it's only chance that the code isn't
> fetching anything from the result of read_info.

What other changes are you referring to?  I don't recall any similar
ones and don't find any in the logs.