Dubious coding in nbtinsert.c

Started by Tom Laneabout 5 years ago3 messageshackers
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

Buildfarm member curculio, which doesn't usually produce
uninitialized-variable warnings, is showing one here:

nbtinsert.c: In function '_bt_doinsert':
nbtinsert.c:411: warning: 'curitemid' may be used uninitialized in this function
nbtinsert.c:411: note: 'curitemid' was declared here

I can see its point: curitemid is set only if !inposting.
While the first two uses of the value are clearly reached
only if !inposting, it's FAR from clear that it's impossible
to reach "ItemIdMarkDead(curitemid);" without a valid value.
Could you clean that up?

regards, tom lane

In reply to: Tom Lane (#1)
Re: Dubious coding in nbtinsert.c

On Thu, Apr 8, 2021 at 12:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Buildfarm member curculio, which doesn't usually produce
uninitialized-variable warnings, is showing one here:

nbtinsert.c: In function '_bt_doinsert':
nbtinsert.c:411: warning: 'curitemid' may be used uninitialized in this function
nbtinsert.c:411: note: 'curitemid' was declared here

I can see its point: curitemid is set only if !inposting.
While the first two uses of the value are clearly reached
only if !inposting, it's FAR from clear that it's impossible
to reach "ItemIdMarkDead(curitemid);" without a valid value.
Could you clean that up?

I'll take care of it shortly.

You had a near-identical complaint about a compiler warning that led
to my commit d64f1cdf2f4 -- that one involved _bt_check_unique()'s
curitup, while this one is about curitemid. While I have no problem
silencing this compiler warning now, I don't see any reason to not
just do the same thing again. Which is to initialize the pointer to
NULL.

--
Peter Geoghegan

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Geoghegan (#2)
Re: Dubious coding in nbtinsert.c

Peter Geoghegan <pg@bowt.ie> writes:

You had a near-identical complaint about a compiler warning that led
to my commit d64f1cdf2f4 -- that one involved _bt_check_unique()'s
curitup, while this one is about curitemid. While I have no problem
silencing this compiler warning now, I don't see any reason to not
just do the same thing again. Which is to initialize the pointer to
NULL.

Works for me; if there is any bug in the logic, we'll get a core dump
and can investigate.

regards, tom lane