Comparison with "true" in source code
There are some "== true" in the codes, but they might not be safe
because all non-zero values are true in C. Is it worth cleaning up them?
src/backend/access/gin/ginget.c(1364):#define GinIsVoidRes(s) (
((GinScanOpaque) scan->opaque)->isVoidRes == true )
src/backend/access/gist/gistproc.c(383): if (allisequal == true && (
src/backend/commands/sequence.c(1107): Assert(new->is_cycled == false
|| new->is_cycled == true);
src/backend/tsearch/regis.c(251): if (mb_strchr((char *) ptr->data,
c) == true)
src/backend/utils/adt/geo_ops.c(3899): for (i = start; i < poly->npts
&& res == true; i++)
src/backend/utils/adt/geo_ops.c(3975): for (i = 0; i < polyb->npts &&
result == true; i++)
src/backend/utils/adt/tsrank.c(112): if (item->prefix == true)
src/backend/utils/adt/tsvector_op.c(628): if (res == false &&
val->prefix == true)
src/include/c.h(475):#define BoolIsValid(boolean) ((boolean) == false
|| (boolean) == true)
src/bin/psql/print.c(832): if (opt_border != 0 ||
format->wrap_right_border == true)
src/interfaces/ecpg/ecpglib/connect.c(168): if (con->autocommit ==
true && strncmp(mode, "off", strlen("off")) == 0)
src/interfaces/ecpg/preproc/ecpg.addons(356): if (compat ==
ECPG_COMPAT_INFORMIX_SE && autocommit == true)
src/interfaces/ecpg/preproc/ecpg.c(310): ptr2ext[3] = (header_mode
== true) ? 'h' : 'c';
src/interfaces/ecpg/preproc/ecpg.c(327): ptr2ext[1] = (header_mode
== true) ? 'h' : 'c';
--
Itagaki Takahiro
On Mon, Nov 01, 2010 at 12:17:02PM +0900, Itagaki Takahiro wrote:
There are some "== true" in the codes, but they might not be safe
because all non-zero values are true in C. Is it worth cleaning up them?
...
src/interfaces/ecpg/ecpglib/connect.c(168): if (con->autocommit ==
true && strncmp(mode, "off", strlen("off")) == 0)
src/interfaces/ecpg/preproc/ecpg.addons(356): if (compat ==
ECPG_COMPAT_INFORMIX_SE && autocommit == true)
src/interfaces/ecpg/preproc/ecpg.c(310): ptr2ext[3] = (header_mode
== true) ? 'h' : 'c';
src/interfaces/ecpg/preproc/ecpg.c(327): ptr2ext[1] = (header_mode
== true) ? 'h' : 'c';
I actually see no reason why these variables are not defined as bool instead of
int, so I changed this. Hopefully I found all of them.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
On Wed, Nov 3, 2010 at 2:19 AM, Michael Meskes <meskes@postgresql.org> wrote:
On Mon, Nov 01, 2010 at 12:17:02PM +0900, Itagaki Takahiro wrote:
There are some "== true" in the codes, but they might not be safe
because all non-zero values are true in C. Is it worth cleaning up them?
Here is a proposed cleanup that replaces "boolean == true" with "boolean".
I didn't touch "== false" unless they are not in pairs of comparisons
with true because comparison with false is a valid C code.
Note that I also changed "boolean != true" in pg_upgrade,
but I didn't change ones in xlog.c because it might check
corrupted fields in control files.
src/interfaces/ecpg/preproc/ecpg.c(310):
ptr2ext[3] = (header_mode == true) ? 'h' : 'c';I actually see no reason why these variables are not defined as bool instead of
int, so I changed this. Hopefully I found all of them.
I added an additional cleanup to 'header_mode' in ecpg; I changed the type
from bool to char to hold 'h' or 'c'. Do you think it is reasonable?
--
Itagaki Takahiro
Attachments:
bool_eq_true_cleanup.patchapplication/octet-stream; name=bool_eq_true_cleanup.patchDownload
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
index d706fc0..519159e 100644
*** a/contrib/pg_upgrade/pg_upgrade.c
--- b/contrib/pg_upgrade/pg_upgrade.c
*************** copy_clog_xlog_xid(void)
*** 254,260 ****
snprintf(old_clog_path, sizeof(old_clog_path), "%s/pg_clog", old_cluster.pgdata);
snprintf(new_clog_path, sizeof(new_clog_path), "%s/pg_clog", new_cluster.pgdata);
! if (rmtree(new_clog_path, true) != true)
pg_log(PG_FATAL, "Unable to delete directory %s\n", new_clog_path);
check_ok();
--- 254,260 ----
snprintf(old_clog_path, sizeof(old_clog_path), "%s/pg_clog", old_cluster.pgdata);
snprintf(new_clog_path, sizeof(new_clog_path), "%s/pg_clog", new_cluster.pgdata);
! if (!rmtree(new_clog_path, true))
pg_log(PG_FATAL, "Unable to delete directory %s\n", new_clog_path);
check_ok();
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 064ac94..34a6cd9 100644
*** a/src/backend/access/gin/ginget.c
--- b/src/backend/access/gin/ginget.c
*************** scanGetItem(IndexScanDesc scan, ItemPoin
*** 1361,1367 ****
}
#define GinIsNewKey(s) ( ((GinScanOpaque) scan->opaque)->keys == NULL )
! #define GinIsVoidRes(s) ( ((GinScanOpaque) scan->opaque)->isVoidRes == true )
Datum
gingetbitmap(PG_FUNCTION_ARGS)
--- 1361,1367 ----
}
#define GinIsNewKey(s) ( ((GinScanOpaque) scan->opaque)->keys == NULL )
! #define GinIsVoidRes(s) ( ((GinScanOpaque) scan->opaque)->isVoidRes )
Datum
gingetbitmap(PG_FUNCTION_ARGS)
diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index 9f6fb34..681ffd2 100644
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
*************** gist_box_picksplit(PG_FUNCTION_ARGS)
*** 380,391 ****
for (i = OffsetNumberNext(FirstOffsetNumber); i <= maxoff; i = OffsetNumberNext(i))
{
cur = DatumGetBoxP(entryvec->vector[i].key);
! if (allisequal == true && (
! pageunion.high.x != cur->high.x ||
! pageunion.high.y != cur->high.y ||
! pageunion.low.x != cur->low.x ||
! pageunion.low.y != cur->low.y
! ))
allisequal = false;
adjustBox(&pageunion, cur);
--- 380,391 ----
for (i = OffsetNumberNext(FirstOffsetNumber); i <= maxoff; i = OffsetNumberNext(i))
{
cur = DatumGetBoxP(entryvec->vector[i].key);
! if (allisequal && (
! pageunion.high.x != cur->high.x ||
! pageunion.high.y != cur->high.y ||
! pageunion.low.x != cur->low.x ||
! pageunion.low.y != cur->low.y
! ))
allisequal = false;
adjustBox(&pageunion, cur);
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 04b0c71..62d1fbf 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*************** init_params(List *options, bool isInit,
*** 1104,1110 ****
if (is_cycled != NULL)
{
new->is_cycled = intVal(is_cycled->arg);
! Assert(new->is_cycled == false || new->is_cycled == true);
}
else if (isInit)
new->is_cycled = false;
--- 1104,1110 ----
if (is_cycled != NULL)
{
new->is_cycled = intVal(is_cycled->arg);
! Assert(BoolIsValid(new->is_cycled));
}
else if (isInit)
new->is_cycled = false;
diff --git a/src/backend/tsearch/regis.c b/src/backend/tsearch/regis.c
index c3114e7..93e2cfd 100644
*** a/src/backend/tsearch/regis.c
--- b/src/backend/tsearch/regis.c
*************** RS_execute(Regis *r, char *str)
*** 244,254 ****
switch (ptr->type)
{
case RSF_ONEOF:
! if (mb_strchr((char *) ptr->data, c) != true)
return false;
break;
case RSF_NONEOF:
! if (mb_strchr((char *) ptr->data, c) == true)
return false;
break;
default:
--- 244,254 ----
switch (ptr->type)
{
case RSF_ONEOF:
! if (!mb_strchr((char *) ptr->data, c))
return false;
break;
case RSF_NONEOF:
! if (mb_strchr((char *) ptr->data, c))
return false;
break;
default:
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index f3b6a38..da2cd9c 100644
*** a/src/backend/utils/adt/geo_ops.c
--- b/src/backend/utils/adt/geo_ops.c
*************** lseg_inside_poly(Point *a, Point *b, POL
*** 3896,3902 ****
t.p[1] = *b;
s.p[0] = poly->p[(start == 0) ? (poly->npts - 1) : (start - 1)];
! for (i = start; i < poly->npts && res == true; i++)
{
Point *interpt;
--- 3896,3902 ----
t.p[1] = *b;
s.p[0] = poly->p[(start == 0) ? (poly->npts - 1) : (start - 1)];
! for (i = start; i < poly->npts && res; i++)
{
Point *interpt;
*************** poly_contain(PG_FUNCTION_ARGS)
*** 3972,3978 ****
s.p[0] = polyb->p[polyb->npts - 1];
result = true;
! for (i = 0; i < polyb->npts && result == true; i++)
{
s.p[1] = polyb->p[i];
result = lseg_inside_poly(s.p, s.p + 1, polya, 0);
--- 3972,3978 ----
s.p[0] = polyb->p[polyb->npts - 1];
result = true;
! for (i = 0; i < polyb->npts && result; i++)
{
s.p[1] = polyb->p[i];
result = lseg_inside_poly(s.p, s.p + 1, polya, 0);
diff --git a/src/backend/utils/adt/tsrank.c b/src/backend/utils/adt/tsrank.c
index b0417de..4b9e938 100644
*** a/src/backend/utils/adt/tsrank.c
--- b/src/backend/utils/adt/tsrank.c
*************** find_wordentry(TSVector t, TSQuery q, Qu
*** 109,115 ****
StopHigh = StopMiddle;
}
! if (item->prefix == true)
{
if (StopLow >= StopHigh)
StopMiddle = StopHigh;
--- 109,115 ----
StopHigh = StopMiddle;
}
! if (item->prefix)
{
if (StopLow >= StopHigh)
StopMiddle = StopHigh;
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 7695097..1920f2a 100644
*** a/src/backend/utils/adt/tsvector_op.c
--- b/src/backend/utils/adt/tsvector_op.c
*************** checkcondition_str(void *checkval, Query
*** 625,631 ****
StopHigh = StopMiddle;
}
! if (res == false && val->prefix == true)
{
/*
* there was a failed exact search, so we should scan further to find
--- 625,631 ----
StopHigh = StopMiddle;
}
! if (!res && val->prefix)
{
/*
* there was a failed exact search, so we should scan further to find
diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index a5007da..d0ef8f9 100644
*** a/src/bin/psql/print.c
--- b/src/bin/psql/print.c
*************** print_aligned_text(const printTableConte
*** 829,835 ****
else
fprintf(fout, "%*s", width_wrap[i], "");
! if (opt_border != 0 || format->wrap_right_border == true)
fputs(!header_done[i] ? format->header_nl_right : " ",
fout);
--- 829,835 ----
else
fprintf(fout, "%*s", width_wrap[i], "");
! if (opt_border != 0 || format->wrap_right_border)
fputs(!header_done[i] ? format->header_nl_right : " ",
fout);
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 3bd6614..1f91878 100644
*** a/src/interfaces/ecpg/ecpglib/connect.c
--- b/src/interfaces/ecpg/ecpglib/connect.c
*************** ECPGsetcommit(int lineno, const char *mo
*** 165,171 ****
ecpg_log("ECPGsetcommit on line %d: action \"%s\"; connection \"%s\"\n", lineno, mode, con->name);
! if (con->autocommit == true && strncmp(mode, "off", strlen("off")) == 0)
{
if (PQtransactionStatus(con->connection) == PQTRANS_IDLE)
{
--- 165,171 ----
ecpg_log("ECPGsetcommit on line %d: action \"%s\"; connection \"%s\"\n", lineno, mode, con->name);
! if (con->autocommit && strncmp(mode, "off", strlen("off")) == 0)
{
if (PQtransactionStatus(con->connection) == PQTRANS_IDLE)
{
*************** ECPGsetcommit(int lineno, const char *mo
*** 176,182 ****
}
con->autocommit = false;
}
! else if (con->autocommit == false && strncmp(mode, "on", strlen("on")) == 0)
{
if (PQtransactionStatus(con->connection) != PQTRANS_IDLE)
{
--- 176,182 ----
}
con->autocommit = false;
}
! else if (!con->autocommit && strncmp(mode, "on", strlen("on")) == 0)
{
if (PQtransactionStatus(con->connection) != PQTRANS_IDLE)
{
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index c39b13c..3b74ba0 100644
*** a/src/interfaces/ecpg/preproc/ecpg.addons
--- b/src/interfaces/ecpg/preproc/ecpg.addons
*************** ECPG: ClosePortalStmtCLOSEcursor_name bl
*** 353,359 ****
}
ECPG: opt_hold block
{
! if (compat == ECPG_COMPAT_INFORMIX_SE && autocommit == true)
$$ = make_str("with hold");
else
$$ = EMPTY;
--- 353,359 ----
}
ECPG: opt_hold block
{
! if (compat == ECPG_COMPAT_INFORMIX_SE && autocommit)
$$ = make_str("with hold");
else
$$ = EMPTY;
diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index 917200a..d1fe1b7 100644
*** a/src/interfaces/ecpg/preproc/ecpg.c
--- b/src/interfaces/ecpg/preproc/ecpg.c
***************
*** 12,18 ****
#include "extern.h"
int ret_value = 0;
! bool autocommit = false,
auto_create_c = false,
system_includes = false,
force_indicator = true,
--- 12,18 ----
#include "extern.h"
int ret_value = 0;
! bool autocommit = false,
auto_create_c = false,
system_includes = false,
force_indicator = true,
*************** main(int argc, char *const argv[])
*** 127,134 ****
int fnr,
c,
out_option = 0;
! bool verbose = false,
! header_mode = false;
struct _include_path *ip;
const char *progname;
char my_exec_path[MAXPGPATH];
--- 127,134 ----
int fnr,
c,
out_option = 0;
! bool verbose = false;
! char header_mode = 'c';
struct _include_path *ip;
const char *progname;
char my_exec_path[MAXPGPATH];
*************** main(int argc, char *const argv[])
*** 196,202 ****
verbose = true;
break;
case 'h':
! header_mode = true;
/* this must include "-c" to make sense */
/* so do not place a "break;" here */
case 'c':
--- 196,202 ----
verbose = true;
break;
case 'h':
! header_mode = 'h';
/* this must include "-c" to make sense */
/* so do not place a "break;" here */
case 'c':
*************** main(int argc, char *const argv[])
*** 307,313 ****
ptr2ext[0] = '.';
ptr2ext[1] = 'p';
ptr2ext[2] = 'g';
! ptr2ext[3] = (header_mode == true) ? 'h' : 'c';
ptr2ext[4] = '\0';
}
--- 307,313 ----
ptr2ext[0] = '.';
ptr2ext[1] = 'p';
ptr2ext[2] = 'g';
! ptr2ext[3] = header_mode;
ptr2ext[4] = '\0';
}
*************** main(int argc, char *const argv[])
*** 324,330 ****
ptr2ext = strrchr(output_filename, '.');
/* make extension = .c resp. .h */
! ptr2ext[1] = (header_mode == true) ? 'h' : 'c';
ptr2ext[2] = '\0';
yyout = fopen(output_filename, PG_BINARY_W);
--- 324,330 ----
ptr2ext = strrchr(output_filename, '.');
/* make extension = .c resp. .h */
! ptr2ext[1] = header_mode;
ptr2ext[2] = '\0';
yyout = fopen(output_filename, PG_BINARY_W);
*************** main(int argc, char *const argv[])
*** 438,444 ****
else
fprintf(yyout, "/* Processed by ecpg (%d.%d.%d) */\n", MAJOR_VERSION, MINOR_VERSION, PATCHLEVEL);
! if (header_mode == false)
{
fprintf(yyout, "/* These include files are added by the preprocessor */\n#include <ecpglib.h>\n#include <ecpgerrno.h>\n#include <sqlca.h>\n");
--- 438,444 ----
else
fprintf(yyout, "/* Processed by ecpg (%d.%d.%d) */\n", MAJOR_VERSION, MINOR_VERSION, PATCHLEVEL);
! if (header_mode == 'c')
{
fprintf(yyout, "/* These include files are added by the preprocessor */\n#include <ecpglib.h>\n#include <ecpgerrno.h>\n#include <sqlca.h>\n");
On Wed, Nov 3, 2010 at 6:45 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
On Wed, Nov 3, 2010 at 2:19 AM, Michael Meskes <meskes@postgresql.org> wrote:
On Mon, Nov 01, 2010 at 12:17:02PM +0900, Itagaki Takahiro wrote:
There are some "== true" in the codes, but they might not be safe
because all non-zero values are true in C. Is it worth cleaning up them?Here is a proposed cleanup that replaces "boolean == true" with "boolean".
I didn't touch "== false" unless they are not in pairs of comparisons
with true because comparison with false is a valid C code.
It looks like you have one or two irrelevant whitespace changes in ecpg.c.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Nov 3, 2010 at 9:45 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
On Wed, Nov 3, 2010 at 2:19 AM, Michael Meskes <meskes@postgresql.org> wrote:
On Mon, Nov 01, 2010 at 12:17:02PM +0900, Itagaki Takahiro wrote:
There are some "== true" in the codes, but they might not be safe
because all non-zero values are true in C. Is it worth cleaning up them?Here is a proposed cleanup that replaces "boolean == true" with "boolean".
I didn't touch "== false" unless they are not in pairs of comparisons
with true because comparison with false is a valid C code.Note that I also changed "boolean != true" in pg_upgrade,
but I didn't change ones in xlog.c because it might check
corrupted fields in control files.src/interfaces/ecpg/preproc/ecpg.c(310):
ptr2ext[3] = (header_mode == true) ? 'h' : 'c';I actually see no reason why these variables are not defined as bool instead of
int, so I changed this. Hopefully I found all of them.I added an additional cleanup to 'header_mode' in ecpg; I changed the type
from bool to char to hold 'h' or 'c'. Do you think it is reasonable?
I looked at this but found that part a bit too clever for its own good.
So committed the rest, plus an additional one-line change to psql's
print.c to avoid making the two accesses to format->wrap_right_pointer
inconsistent with each other.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Nov 15, 2010 at 11:13, Robert Haas <robertmhaas@gmail.com> wrote:
I added an additional cleanup to 'header_mode' in ecpg; I changed the type
from bool to char to hold 'h' or 'c'. Do you think it is reasonable?I looked at this but found that part a bit too clever for its own good.
So committed the rest, plus an additional one-line change to psql's
print.c to avoid making the two accesses to format->wrap_right_pointer
inconsistent with each other.
Thanks!
--
Itagaki Takahiro