Wrong dead return value in jsonb_utils.c
Returning -1 from a function with bool as return value is the same as
returning true. Now, the code is dead (since elog(ERROR, ...) does not
return) so it doesn't matter to the compiler, but changing to false is less
confusing for the programmer. Appologies if this is seen as unnecessary
churn.
The same code is present since 9.4, but perhaps it's not really worth
backporting since it is more of an aesthetic change?
Attachments:
0001-Fix-dead-code-return-value-in-jsonb_util.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-dead-code-return-value-in-jsonb_util.patchDownload+1-2
On Sun, May 12, 2019 at 03:18:08AM +0200, Rikard Falkeborn wrote:
Returning -1 from a function with bool as return value is the same as
returning true. Now, the code is dead (since elog(ERROR, ...) does not
return) so it doesn't matter to the compiler, but changing to false is less
confusing for the programmer. Appologies if this is seen as unnecessary
churn.The same code is present since 9.4, but perhaps it's not really worth
backporting since it is more of an aesthetic change?
This is an aesthetic change in the fact that elog(ERROR) would not
cause -1 to be returned, still I agree that it is cleaner to do things
the way your patch does. And the origin of the issue is I think that
the code of equalsJsonbScalarValue() has been copy-pasted from
compareJsonbScalarValue().
As that's really cosmetic, I would just change that on HEAD, or
perhaps others feel differently?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
As that's really cosmetic, I would just change that on HEAD, or
perhaps others feel differently?
+1 for a HEAD-only change. I think the only really good arguments
for back-patching would be if this were causing compiler warnings
(but we've seen none) or if we thought it would likely lead to
hazards for back-patching future bug fixes (but the adjacent lines
seem unlikely to change).
regards, tom lane
On Sun, May 12, 2019 at 10:15:05AM -0400, Tom Lane wrote:
+1 for a HEAD-only change. I think the only really good arguments
for back-patching would be if this were causing compiler warnings
(but we've seen none) or if we thought it would likely lead to
hazards for back-patching future bug fixes (but the adjacent lines
seem unlikely to change).
If it were to generate warnings, we would have already caught them as
this comes from 1171dbd. Committed to HEAD.
--
Michael