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

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.

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.

That's exactly why, instead of proposing a patch, I asked a question.

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 fortunately have been fixed by other
committers.
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.

regards,
Ranier Vilela

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 fortunately have been fixed by other committers.
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.

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/

Em seg., 30 de mar. de 2020 às 05:16, Michael Paquier <michael@paquier.xyz>
escreveu:

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.

I have certainly taken this advice seriously, since I have received all
kinds of say, "words of discouragement".
I understand perfectly that the list is very busy and perhaps the patience
with mistakes is very little, but these attitudes do not help new people to
work here.
I don't get paid to work with PostgreSQL, so consideration and recognition
are the only rewards for now.

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.

That is the question. Looking only at the code, there is no way to know
immediately, that there is nothing wrong. Not even a comment warning.
That's what the tool asked for, ask if there's really nothing wrong.

regards,
Ranier Vilela

Em seg., 30 de mar. de 2020 às 06:06, Magnus Hagander <magnus@hagander.net>
escreveu:

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.

Of course, now we all know..

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 fortunately have been fixed by
other committers.

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.

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.

that's what you said, it could be a big problem, if it were the case of
copy-past error.
I do not consider it a false positive, since the tool did not claim it was
a bug, she warned and asked to question.

regards,
Ranier Vilela