pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
11+ messages / 6 participants
[nested] [flat]
* BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
@ 2023-12-06 04:49 PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
0 siblings, 1 reply; 11+ messages in thread
From: PG Bug reporting form @ 2023-12-06 04:49 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: rekgrpth@gmail.com
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)
...
```
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
@ 2023-12-06 13:55 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Laurenz Albe @ 2023-12-06 13:55 UTC (permalink / raw)
To: rekgrpth@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
@ 2023-12-06 14:44 ` RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
0 siblings, 1 reply; 11+ messages in thread
From: RekGRpth @ 2023-12-06 14:44 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: pgsql-bugs@lists.postgresql.org
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
Attachments:
tzp.diff (text/x-patch, 4.1K) download
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 99bdc94d6d..7783316596 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -1790,8 +1790,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
bool haveTextMonth = false;
bool is2digits = false;
bool bc = false;
- int t = 0;
- int *tzp = &t;
+ int tzp = 0;
/***
* We'll insist on at least all of the date fields, but initialize the
@@ -1804,8 +1803,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*fsec = 0;
/* don't know daylight savings time status apriori */
tm->tm_isdst = -1;
- if (tzp != NULL)
- *tzp = 0;
for (i = 0; i < nf; i++)
{
@@ -1822,16 +1819,13 @@ DecodeDateTime(char **field, int *ftype, int nf,
char *cp;
int jday;
- if (tzp == NULL)
- return -1;
-
jday = strtoint(field[i], &cp, 10);
if (*cp != '-')
return -1;
j2date(jday, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
/* Get the time zone from the end of the string */
- if (DecodeTimezone(cp, tzp) != 0)
+ if (DecodeTimezone(cp, &tzp) != 0)
return -1;
tmask = DTK_DATE_M | DTK_TIME_M | DTK_M(TZ);
@@ -1847,10 +1841,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
else if (((fmask & DTK_DATE_M) == DTK_DATE_M)
|| (ptype != 0))
{
- /* No time zone accepted? Then quit... */
- if (tzp == NULL)
- return -1;
-
if (isdigit((unsigned char) *field[i]) || ptype != 0)
{
char *cp;
@@ -1875,7 +1865,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
return -1;
/* Get the time zone from the end of the string */
- if (DecodeTimezone(cp, tzp) != 0)
+ if (DecodeTimezone(cp, &tzp) != 0)
return -1;
*cp = '\0';
@@ -1895,7 +1885,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
}
else
{
- if (DecodePosixTimezone(field[i], tzp) != 0)
+ if (DecodePosixTimezone(field[i], &tzp) != 0)
return -1;
ftype[i] = DTK_TZ;
@@ -1924,9 +1914,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
{
int tz;
- if (tzp == NULL)
- return -1;
-
if (DecodeTimezone(field[i], &tz) != 0)
return -1;
@@ -1938,12 +1925,12 @@ DecodeDateTime(char **field, int *ftype, int nf,
ftype[i - 1] == DTK_TZ &&
isalpha((unsigned char) *field[i - 1]))
{
- *tzp -= tz;
+ tzp -= tz;
tmask = 0;
}
else
{
- *tzp = tz;
+ tzp = tz;
tmask = DTK_M(TZ);
}
}
@@ -2037,7 +2024,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
case DTK_TZ:
tmask = DTK_M(TZ);
- if (DecodeTimezone(field[i], tzp) != 0)
+ if (DecodeTimezone(field[i], &tzp) != 0)
return -1;
break;
@@ -2173,8 +2160,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
tm->tm_hour = 0;
tm->tm_min = 0;
tm->tm_sec = 0;
- if (tzp != NULL)
- *tzp = 0;
+ tzp = 0;
break;
default:
@@ -2207,9 +2193,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(DTZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
- *tzp -= val;
+ tzp -= val;
break;
case DTZ:
@@ -2220,17 +2204,13 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(TZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
- *tzp = -val;
+ tzp = -val;
ftype[i] = DTK_TZ;
break;
case TZ:
tm->tm_isdst = 0;
- if (tzp == NULL)
- return -1;
- *tzp = -val;
+ tzp = -val;
ftype[i] = DTK_TZ;
break;
@@ -2336,7 +2316,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
* result afterwards anyway so we only check for this error: daylight
* savings time modifier but no standard timezone?
*/
- if ((fmask & DTK_DATE_M) == DTK_DATE_M && tzp != NULL && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
+ if ((fmask & DTK_DATE_M) == DTK_DATE_M && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
return -1;
}
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
@ 2023-12-06 16:38 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Laurenz Albe @ 2023-12-06 16:38 UTC (permalink / raw)
To: RekGRpth <rekgrpth@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
@ 2023-12-06 16:41 ` RekGRpth <rekgrpth@gmail.com>
2023-12-06 21:01 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-07 01:51 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Richard Guo <guofenglinux@gmail.com>
0 siblings, 2 replies; 11+ messages in thread
From: RekGRpth @ 2023-12-06 16:41 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: pgsql-bugs@lists.postgresql.org
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
>
Attachments:
tzp2.diff (text/x-patch, 2.5K) download
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 99bdc94d6d..c46403b809 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -1804,8 +1804,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*fsec = 0;
/* don't know daylight savings time status apriori */
tm->tm_isdst = -1;
- if (tzp != NULL)
- *tzp = 0;
for (i = 0; i < nf; i++)
{
@@ -1822,9 +1820,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
char *cp;
int jday;
- if (tzp == NULL)
- return -1;
-
jday = strtoint(field[i], &cp, 10);
if (*cp != '-')
return -1;
@@ -1847,10 +1842,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
else if (((fmask & DTK_DATE_M) == DTK_DATE_M)
|| (ptype != 0))
{
- /* No time zone accepted? Then quit... */
- if (tzp == NULL)
- return -1;
-
if (isdigit((unsigned char) *field[i]) || ptype != 0)
{
char *cp;
@@ -1924,9 +1915,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
{
int tz;
- if (tzp == NULL)
- return -1;
-
if (DecodeTimezone(field[i], &tz) != 0)
return -1;
@@ -2173,8 +2161,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
tm->tm_hour = 0;
tm->tm_min = 0;
tm->tm_sec = 0;
- if (tzp != NULL)
- *tzp = 0;
+ *tzp = 0;
break;
default:
@@ -2207,8 +2194,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(DTZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
*tzp -= val;
break;
@@ -2220,16 +2205,12 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(TZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
*tzp = -val;
ftype[i] = DTK_TZ;
break;
case TZ:
tm->tm_isdst = 0;
- if (tzp == NULL)
- return -1;
*tzp = -val;
ftype[i] = DTK_TZ;
break;
@@ -2336,7 +2317,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
* result afterwards anyway so we only check for this error: daylight
* savings time modifier but no standard timezone?
*/
- if ((fmask & DTK_DATE_M) == DTK_DATE_M && tzp != NULL && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
+ if ((fmask & DTK_DATE_M) == DTK_DATE_M && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
return -1;
}
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
@ 2023-12-06 21:01 ` Laurenz Albe <laurenz.albe@cybertec.at>
1 sibling, 0 replies; 11+ messages in thread
From: Laurenz Albe @ 2023-12-06 21:01 UTC (permalink / raw)
To: RekGRpth <rekgrpth@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Wed, 2023-12-06 at 21:41 +0500, RekGRpth wrote:
> Fixed!
That patch looks good to me.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
@ 2023-12-07 01:51 ` Richard Guo <guofenglinux@gmail.com>
2023-12-07 02:28 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-07 02:32 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Michael Paquier <michael@paquier.xyz>
1 sibling, 2 replies; 11+ messages in thread
From: Richard Guo @ 2023-12-07 01:51 UTC (permalink / raw)
To: RekGRpth <rekgrpth@gmail.com>; +Cc: Laurenz Albe <laurenz.albe@cybertec.at>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-07 01:51 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Richard Guo <guofenglinux@gmail.com>
@ 2023-12-07 02:28 ` RekGRpth <rekgrpth@gmail.com>
1 sibling, 0 replies; 11+ messages in thread
From: RekGRpth @ 2023-12-07 02:28 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Laurenz Albe <laurenz.albe@cybertec.at>; pgsql-bugs@lists.postgresql.org
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
Attachments:
tz.diff (text/x-patch, 4.1K) download
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 99bdc94d6d..f532185079 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -1790,8 +1790,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
bool haveTextMonth = false;
bool is2digits = false;
bool bc = false;
- int t = 0;
- int *tzp = &t;
+ int tz = 0;
/***
* We'll insist on at least all of the date fields, but initialize the
@@ -1804,8 +1803,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*fsec = 0;
/* don't know daylight savings time status apriori */
tm->tm_isdst = -1;
- if (tzp != NULL)
- *tzp = 0;
for (i = 0; i < nf; i++)
{
@@ -1822,16 +1819,13 @@ DecodeDateTime(char **field, int *ftype, int nf,
char *cp;
int jday;
- if (tzp == NULL)
- return -1;
-
jday = strtoint(field[i], &cp, 10);
if (*cp != '-')
return -1;
j2date(jday, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
/* Get the time zone from the end of the string */
- if (DecodeTimezone(cp, tzp) != 0)
+ if (DecodeTimezone(cp, &tz) != 0)
return -1;
tmask = DTK_DATE_M | DTK_TIME_M | DTK_M(TZ);
@@ -1847,10 +1841,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
else if (((fmask & DTK_DATE_M) == DTK_DATE_M)
|| (ptype != 0))
{
- /* No time zone accepted? Then quit... */
- if (tzp == NULL)
- return -1;
-
if (isdigit((unsigned char) *field[i]) || ptype != 0)
{
char *cp;
@@ -1875,7 +1865,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
return -1;
/* Get the time zone from the end of the string */
- if (DecodeTimezone(cp, tzp) != 0)
+ if (DecodeTimezone(cp, &tz) != 0)
return -1;
*cp = '\0';
@@ -1895,7 +1885,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
}
else
{
- if (DecodePosixTimezone(field[i], tzp) != 0)
+ if (DecodePosixTimezone(field[i], &tz) != 0)
return -1;
ftype[i] = DTK_TZ;
@@ -1922,11 +1912,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
case DTK_TZ:
{
- int tz;
-
- if (tzp == NULL)
- return -1;
-
if (DecodeTimezone(field[i], &tz) != 0)
return -1;
@@ -1937,15 +1922,9 @@ DecodeDateTime(char **field, int *ftype, int nf,
if (i > 0 && (fmask & DTK_M(TZ)) != 0 &&
ftype[i - 1] == DTK_TZ &&
isalpha((unsigned char) *field[i - 1]))
- {
- *tzp -= tz;
tmask = 0;
- }
else
- {
- *tzp = tz;
tmask = DTK_M(TZ);
- }
}
break;
@@ -2037,7 +2016,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
case DTK_TZ:
tmask = DTK_M(TZ);
- if (DecodeTimezone(field[i], tzp) != 0)
+ if (DecodeTimezone(field[i], &tz) != 0)
return -1;
break;
@@ -2173,8 +2152,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
tm->tm_hour = 0;
tm->tm_min = 0;
tm->tm_sec = 0;
- if (tzp != NULL)
- *tzp = 0;
break;
default:
@@ -2207,9 +2184,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(DTZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
- *tzp -= val;
break;
case DTZ:
@@ -2220,17 +2194,11 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(TZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
- *tzp = -val;
ftype[i] = DTK_TZ;
break;
case TZ:
tm->tm_isdst = 0;
- if (tzp == NULL)
- return -1;
- *tzp = -val;
ftype[i] = DTK_TZ;
break;
@@ -2336,7 +2304,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
* result afterwards anyway so we only check for this error: daylight
* savings time modifier but no standard timezone?
*/
- if ((fmask & DTK_DATE_M) == DTK_DATE_M && tzp != NULL && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
+ if ((fmask & DTK_DATE_M) == DTK_DATE_M && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
return -1;
}
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-07 01:51 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Richard Guo <guofenglinux@gmail.com>
@ 2023-12-07 02:32 ` Michael Paquier <michael@paquier.xyz>
2023-12-07 02:39 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-07 04:05 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 11+ messages in thread
From: Michael Paquier @ 2023-12-07 02:32 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: RekGRpth <rekgrpth@gmail.com>; Laurenz Albe <laurenz.albe@cybertec.at>; pgsql-bugs@lists.postgresql.org
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-07 01:51 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Richard Guo <guofenglinux@gmail.com>
2023-12-07 02:32 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Michael Paquier <michael@paquier.xyz>
@ 2023-12-07 02:39 ` RekGRpth <rekgrpth@gmail.com>
1 sibling, 0 replies; 11+ messages in thread
From: RekGRpth @ 2023-12-07 02:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Richard Guo <guofenglinux@gmail.com>; Laurenz Albe <laurenz.albe@cybertec.at>; pgsql-bugs@lists.postgresql.org
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
Attachments:
rm_tzp.diff (text/x-patch, 4.5K) download
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 99bdc94d6d..97800a51c8 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -1530,7 +1530,8 @@ DecodeTimezone(char *str, int *tzp)
if (*str == '-')
tz = -tz;
- *tzp = -tz;
+ if (tzp != NULL)
+ *tzp = -tz;
return *cp != '\0';
} /* DecodeTimezone() */
@@ -1566,7 +1567,8 @@ DecodePosixTimezone(char *str, int *tzp)
{
case DTZ:
case TZ:
- *tzp = -(val + tz);
+ if (tzp != NULL)
+ *tzp = -(val + tz);
break;
default:
@@ -1790,8 +1792,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
bool haveTextMonth = false;
bool is2digits = false;
bool bc = false;
- int t = 0;
- int *tzp = &t;
/***
* We'll insist on at least all of the date fields, but initialize the
@@ -1804,8 +1804,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*fsec = 0;
/* don't know daylight savings time status apriori */
tm->tm_isdst = -1;
- if (tzp != NULL)
- *tzp = 0;
for (i = 0; i < nf; i++)
{
@@ -1822,16 +1820,13 @@ DecodeDateTime(char **field, int *ftype, int nf,
char *cp;
int jday;
- if (tzp == NULL)
- return -1;
-
jday = strtoint(field[i], &cp, 10);
if (*cp != '-')
return -1;
j2date(jday, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
/* Get the time zone from the end of the string */
- if (DecodeTimezone(cp, tzp) != 0)
+ if (DecodeTimezone(cp, NULL) != 0)
return -1;
tmask = DTK_DATE_M | DTK_TIME_M | DTK_M(TZ);
@@ -1847,10 +1842,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
else if (((fmask & DTK_DATE_M) == DTK_DATE_M)
|| (ptype != 0))
{
- /* No time zone accepted? Then quit... */
- if (tzp == NULL)
- return -1;
-
if (isdigit((unsigned char) *field[i]) || ptype != 0)
{
char *cp;
@@ -1875,7 +1866,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
return -1;
/* Get the time zone from the end of the string */
- if (DecodeTimezone(cp, tzp) != 0)
+ if (DecodeTimezone(cp, NULL) != 0)
return -1;
*cp = '\0';
@@ -1895,7 +1886,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
}
else
{
- if (DecodePosixTimezone(field[i], tzp) != 0)
+ if (DecodePosixTimezone(field[i], NULL) != 0)
return -1;
ftype[i] = DTK_TZ;
@@ -1922,12 +1913,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
case DTK_TZ:
{
- int tz;
-
- if (tzp == NULL)
- return -1;
-
- if (DecodeTimezone(field[i], &tz) != 0)
+ if (DecodeTimezone(field[i], NULL) != 0)
return -1;
/*
@@ -1937,15 +1923,9 @@ DecodeDateTime(char **field, int *ftype, int nf,
if (i > 0 && (fmask & DTK_M(TZ)) != 0 &&
ftype[i - 1] == DTK_TZ &&
isalpha((unsigned char) *field[i - 1]))
- {
- *tzp -= tz;
tmask = 0;
- }
else
- {
- *tzp = tz;
tmask = DTK_M(TZ);
- }
}
break;
@@ -2037,7 +2017,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
case DTK_TZ:
tmask = DTK_M(TZ);
- if (DecodeTimezone(field[i], tzp) != 0)
+ if (DecodeTimezone(field[i], NULL) != 0)
return -1;
break;
@@ -2173,8 +2153,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
tm->tm_hour = 0;
tm->tm_min = 0;
tm->tm_sec = 0;
- if (tzp != NULL)
- *tzp = 0;
break;
default:
@@ -2207,9 +2185,6 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(DTZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
- *tzp -= val;
break;
case DTZ:
@@ -2220,17 +2195,11 @@ DecodeDateTime(char **field, int *ftype, int nf,
*/
tmask |= DTK_M(TZ);
tm->tm_isdst = 1;
- if (tzp == NULL)
- return -1;
- *tzp = -val;
ftype[i] = DTK_TZ;
break;
case TZ:
tm->tm_isdst = 0;
- if (tzp == NULL)
- return -1;
- *tzp = -val;
ftype[i] = DTK_TZ;
break;
@@ -2336,7 +2305,7 @@ DecodeDateTime(char **field, int *ftype, int nf,
* result afterwards anyway so we only check for this error: daylight
* savings time modifier but no standard timezone?
*/
- if ((fmask & DTK_DATE_M) == DTK_DATE_M && tzp != NULL && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
+ if ((fmask & DTK_DATE_M) == DTK_DATE_M && !(fmask & DTK_M(TZ)) && (fmask & DTK_M(DTZMOD)))
return -1;
}
* Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c RekGRpth <rekgrpth@gmail.com>
2023-12-07 01:51 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Richard Guo <guofenglinux@gmail.com>
2023-12-07 02:32 ` Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c Michael Paquier <michael@paquier.xyz>
@ 2023-12-07 04:05 ` Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 11+ messages in thread
From: Tom Lane @ 2023-12-07 04:05 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Richard Guo <guofenglinux@gmail.com>; RekGRpth <rekgrpth@gmail.com>; Laurenz Albe <laurenz.albe@cybertec.at>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-12-07 04:05 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-12-06 04:49 BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c PG Bug reporting form <noreply@postgresql.org>
2023-12-06 13:55 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 14:44 ` RekGRpth <rekgrpth@gmail.com>
2023-12-06 16:38 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-06 16:41 ` RekGRpth <rekgrpth@gmail.com>
2023-12-06 21:01 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-12-07 01:51 ` Richard Guo <guofenglinux@gmail.com>
2023-12-07 02:28 ` RekGRpth <rekgrpth@gmail.com>
2023-12-07 02:32 ` Michael Paquier <michael@paquier.xyz>
2023-12-07 02:39 ` RekGRpth <rekgrpth@gmail.com>
2023-12-07 04:05 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox