Обсуждение: Possible copy and past error? (\usr\backend\commands\analyze.c)

Ranier Vilela <ranier.vf@gmail.com> writes:
> Can someone check if there is a copy and paste error, at file:
> \usr\backend\commands\analyze.c, at lines 2225 and 2226?
> int num_mcv = stats->attr->attstattarget;
> int num_bins = stats->attr->attstattarget;

No, that's intentional I believe.  Those are independent variables
that just happen to start out with the same value.

> If they really are the same values, it could be changed to:

> int num_mcv = stats->attr->attstattarget;
> int num_bins = num_mcv;

That would make it look like they are interdependent, which they are not.

> To silence this alert.

If you have a tool that complains about that coding, I think the
tool needs a solid whack upside the head.  There's nothing wrong
with the code, and it clearly expresses the intent, which the other
way doesn't.  (Or in other words: it's the compiler's job to
optimize away the duplicate fetch.  Not the programmer's.)

            regards, tom lane

On Sat, Mar 28, 2020 at 07:48:22AM -0300, Ranier Vilela wrote:
> I completely disagree. My tools have proven their worth, including finding
> serious errors in the code, which fortunately have been fixed by other
> committers.

FWIW, I think that the rule to always take Coverity's reports with a
pinch of salt applies for any report.

> When issuing this alert, the tool does not value judgment regarding
> performance or optimization, but it does an excellent job of finding
> similar patterns in adjacent lines, and the only thing it asked for was to
> be asked if this was really the case. original author's intention.

The code context matters a lot, but here let's leave this code alone.
There is nothing wrong with it.
--
Michael

On Sat, Mar 28, 2020 at 11:49 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em sex., 27 de mar. de 2020 às 20:49, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>>
>> Ranier Vilela <ranier.vf@gmail.com> writes:
>> > Can someone check if there is a copy and paste error, at file:
>> > \usr\backend\commands\analyze.c, at lines 2225 and 2226?
>> > int num_mcv = stats->attr->attstattarget;
>> > int num_bins = stats->attr->attstattarget;
>>
>> No, that's intentional I believe.  Those are independent variables
>> that just happen to start out with the same value.
>
> Neither you nor I can say with 100% certainty that the original author's intention.

Given that Tom is the original author, I think it's a lot more likely
that he knows what the original authors intention was. It's certainly
been a few years, so it probably isn't 100%, but the likelihood is
pretty good.


>> > To silence this alert.
>>
>> If you have a tool that complains about that coding, I think the
>> tool needs a solid whack upside the head.  There's nothing wrong
>> with the code, and it clearly expresses the intent, which the other
>> way doesn't.  (Or in other words: it's the compiler's job to
>> optimize away the duplicate fetch.  Not the programmer's.)
>
> I completely disagree. My tools have proven their worth, including finding serious errors in the code, which
fortunatelyhave been fixed by other committers. 
> When issuing this alert, the tool does not value judgment regarding performance or optimization, but it does an
excellentjob of finding similar patterns in adjacent lines, and the only thing it asked for was to be asked if this was
reallythe case. original author's intention. 

All tools will give false positives. This simply seems one of those --
it certainly could have been indicating a problem, but in this case it
didn't.

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/