Обсуждение: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
От
PG Bug reporting form
Дата:
The following bug has been logged on the website: Bug reference: 18230 Logged by: RekGRpth Email address: rekgrpth@gmail.com PostgreSQL version: 16.1 Operating system: all Description: All versions of PostgreSQL has redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c in DecodeDateTime function. ```c ... int t = 0; int *tzp = &t; ... if (tzp != NULL) ... if (tzp == NULL) ... ```
On Wed, 2023-12-06 at 04:49 +0000, PG Bug reporting form wrote: > The following bug has been logged on the website: > > Bug reference: 18230 > Logged by: RekGRpth > Email address: rekgrpth@gmail.com > PostgreSQL version: 16.1 > Operating system: all > Description: > > All versions of PostgreSQL has redundant comparison of a local variable > 'tzp' address with a NULL value at dt_common.c in DecodeDateTime function. > > ```c > ... > int t = 0; > int *tzp = &t; > ... > if (tzp != NULL) > ... > > if (tzp == NULL) That's not really a bug, but should certainly be improved. At fault is commit 635a0b9a864, which removed "tzp" as a parameter from "DecodeDateTime" and replaced it with a constant pointer to 0, when it should have done more, like remove the variable and its uses. Do you want to write a patch? Yours, Laurenz Albe
something like? ср, 6 дек. 2023 г. в 18:55, Laurenz Albe <laurenz.albe@cybertec.at>: > > On Wed, 2023-12-06 at 04:49 +0000, PG Bug reporting form wrote: > > The following bug has been logged on the website: > > > > Bug reference: 18230 > > Logged by: RekGRpth > > Email address: rekgrpth@gmail.com > > PostgreSQL version: 16.1 > > Operating system: all > > Description: > > > > All versions of PostgreSQL has redundant comparison of a local variable > > 'tzp' address with a NULL value at dt_common.c in DecodeDateTime function. > > > > ```c > > ... > > int t = 0; > > int *tzp = &t; > > ... > > if (tzp != NULL) > > ... > > > > if (tzp == NULL) > > That's not really a bug, but should certainly be improved. > > At fault is commit 635a0b9a864, which removed "tzp" as a parameter from > "DecodeDateTime" and replaced it with a constant pointer to 0, when it > should have done more, like remove the variable and its uses. > > Do you want to write a patch? > > Yours, > Laurenz Albe
Вложения
On Wed, 2023-12-06 at 19:44 +0500, RekGRpth wrote: > something like? Yes, perhaps - although the variable name "tzp" = "time zone pointer" is not appropriate any more. Perhaps the "tzp" variable could be left as it is, and only the comparisons "if (tzp == NULL)" be removed. Yours, Laurenz Albe
Fixed! ср, 6 дек. 2023 г. в 21:38, Laurenz Albe <laurenz.albe@cybertec.at>: > > On Wed, 2023-12-06 at 19:44 +0500, RekGRpth wrote: > > something like? > > Yes, perhaps - although the variable name "tzp" = "time zone pointer" > is not appropriate any more. > > Perhaps the "tzp" variable could be left as it is, and only the > comparisons "if (tzp == NULL)" be removed. > > Yours, > Laurenz Albe >
Вложения
On Wed, 2023-12-06 at 21:41 +0500, RekGRpth wrote: > Fixed! That patch looks good to me. Yours, Laurenz Albe
On Thu, Dec 7, 2023 at 12:41 AM RekGRpth <rekgrpth@gmail.com> wrote:
Fixed!
Nice catch.
There are several places in DecodeDateTime() where the value of '*tzp'
is changed. It seems to me that these assignments are unnecessary since
the value of '*tzp' is not utilized anywhere in the code. Can we also
remove these assignments?
Thanks
Richard
There are several places in DecodeDateTime() where the value of '*tzp'
is changed. It seems to me that these assignments are unnecessary since
the value of '*tzp' is not utilized anywhere in the code. Can we also
remove these assignments?
Thanks
Richard
Fixed! чт, 7 дек. 2023 г. в 06:52, Richard Guo <guofenglinux@gmail.com>: > > > On Thu, Dec 7, 2023 at 12:41 AM RekGRpth <rekgrpth@gmail.com> wrote: >> >> Fixed! > > > Nice catch. > > There are several places in DecodeDateTime() where the value of '*tzp' > is changed. It seems to me that these assignments are unnecessary since > the value of '*tzp' is not utilized anywhere in the code. Can we also > remove these assignments? > > Thanks > Richard
Вложения
On Thu, Dec 07, 2023 at 09:51:57AM +0800, Richard Guo wrote: > There are several places in DecodeDateTime() where the value of '*tzp' > is changed. It seems to me that these assignments are unnecessary since > the value of '*tzp' is not utilized anywhere in the code. Can we also > remove these assignments? *tzp could be given to DecodeTimezone() and manipulated inside it, but you are right that we don't use it at all in the DecodeDateTime() path. Other callers of DecodeTimezone() may depend on the changes done inside it, though. Perhaps DecodeTimezone() should be extended so as it can accept a NULL value and use that in DecodeDateTime(), eliminating the need for *tzp entirely? -- Michael
Вложения
Fixed! чт, 7 дек. 2023 г. в 07:32, Michael Paquier <michael@paquier.xyz>: > > On Thu, Dec 07, 2023 at 09:51:57AM +0800, Richard Guo wrote: > > There are several places in DecodeDateTime() where the value of '*tzp' > > is changed. It seems to me that these assignments are unnecessary since > > the value of '*tzp' is not utilized anywhere in the code. Can we also > > remove these assignments? > > *tzp could be given to DecodeTimezone() and manipulated inside it, but > you are right that we don't use it at all in the DecodeDateTime() > path. Other callers of DecodeTimezone() may depend on the changes > done inside it, though. Perhaps DecodeTimezone() should be extended > so as it can accept a NULL value and use that in DecodeDateTime(), > eliminating the need for *tzp entirely? > -- > Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > *tzp could be given to DecodeTimezone() and manipulated inside it, but > you are right that we don't use it at all in the DecodeDateTime() > path. Other callers of DecodeTimezone() may depend on the changes > done inside it, though. Perhaps DecodeTimezone() should be extended > so as it can accept a NULL value and use that in DecodeDateTime(), > eliminating the need for *tzp entirely? TBH, I think our best path here is to leave well enough alone. The main impact of the proposed changes is to make ecpg's copy of the datetime parsing code diverge even further from the backend's copy. I don't think that these changes buy anything meaningful, and what they will do is make it harder to reunify that logic, which somebody will probably try to do someday. (The recent changes to support non-error-throwing returns from input functions probably made that task notably easier, BTW.) regards, tom lane