I've occasionally griped that I do not like the coding practice of
writing
	if (strcmp(foo, bar))
to mean
	if (strcmp(foo, bar) != 0)
or its inverse
	if (!strcmp(foo, bar))
to mean
	if (strcmp(foo, bar) == 0)

My past objection to this has been purely stylistic: it's too easy
to read these constructs backwards, eg to think "!strcmp()" means
"not equal".  However, I've now had my nose rubbed in the fact that
this habit is actually dangerous.

Up till just now, ruleutils.c contained code like this:

        bool        tell_as = FALSE;

        /* Check if we must say AS ... */
        if (!IsA(tle->expr, Var))
            tell_as = strcmp(tle->resdom->resname, "?column?");

	/* more code... */

        if (tell_as)
            /* do something */

This is subtly wrong, because it will work as intended on many
platforms.  But on some platforms, strcmp is capable of yielding
values that are not 0 but whose low 8 bits are all 0.  Stuff that
into a char-sized "bool" variable, and all of a sudden it's zero,
reversing the intended behavior of the test.

Correct, portable coding of course is

            tell_as = strcmp(tle->resdom->resname, "?column?") != 0;

This error would not have happened if the author of this code had
been in the habit of regarding strcmp's result as something to compare
against 0, rather than as equivalent to a boolean value.  So, I assert
that the above-mentioned coding practice is dangerous, because it can
lead you to do things that aren't portable.

I'm not planning to engage in a wholesale search-and-destroy mission
for misuses of strcmp and friends just at the moment, but maybe someone
should --- we may have comparable portability bugs elsewhere.  In any
case I suggest we avoid this coding practice in future.

			regards, tom lane

Thu, 06 Jul 2000 19:03:57 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Memo on coding practices: strcmp() does not yield bool
Tom Lane wrote:
> 
> I've occasionally griped that I do not like the coding practice of
> writing
>         
> to mean
>         if (strcmp(foo, bar) != 0)
> ...
> My past objection to this has been purely stylistic: it's too easy
> to read these constructs backwards, eg to think "!strcmp()" means
> "not equal".  However, I've now had my nose rubbed in the fact that
> this habit is actually dangerous.
> 
> Up till just now, ruleutils.c contained code like this:
> 
>         bool        tell_as = FALSE;
> 
>         /* Check if we must say AS ... */
>         if (!IsA(tle->expr, Var))
>             tell_as = strcmp(tle->resdom->resname, "?column?");
> 
>         /* more code... */
> 
>         if (tell_as)
>             /* do something */
> 
> This is subtly wrong, because it will work as intended on many
> platforms.  But on some platforms, strcmp is capable of yielding
> values that are not 0 but whose low 8 bits are all 0.  Stuff that
> into a char-sized "bool" variable, and all of a sudden it's zero,
> reversing the intended behavior of the test.

I see your examples demonstrate the danger of inappropriate or
inattentive type conversion (e.g., splicing an int into a char), but I'm
missing the danger you see, beyond a style offense, of "if (strcmp(foo,
bar))"?

Regards,
Ed Loehr

Thu, 06 Jul 2000 18:37:37 -0500
Ed Loehr <eloehr@austin.rr.com>
Re: Memo on coding practices: strcmp() does not yield bool
Tom Lane wrote:
> I've occasionally griped that I do not like the coding practice of
> writing
>    if (strcmp(foo, bar))
> to mean
>    if (strcmp(foo, bar) != 0)
>
> [...]
>
> Up till just now, ruleutils.c contained code like this:
>
>         bool        tell_as = FALSE;
>
>         /* Check if we must say AS ... */
>         if (!IsA(tle->expr, Var))
>             tell_as = strcmp(tle->resdom->resname, "?column?");
>
> [...]

    Yeah, blame me for that one.

    Oh  boy.  Originally  I  wrote  ruleutils.c  as  a proof that
    rewrite rules "can" tell what the  original  rule  (or  view)
    looked  like.  Someone  called it a "magic piece of software"
    and we adopted it as a useful thing to dump views  and  rules
    (what  we  wheren't  able before).  Now you blame me for it's
    uglyness.

    Well done! I was so proude beeing able to  show  how  much  I
    knew  about rewrite rules by coding a reverse interpreter for
    them. Just that I was too lazy to code it in a good style.

    Remind me on it if I ever ask "what should I do next".

    :-)


Jan

PS: I'm sure Tom can read between the lines what I really  wanted
    to  say. For anyone else: The functionality of ruleutils.c is
    checked in the regression suite. None of the  ports  reported
    problems  so  far,  so  the requested style fixes are more or
    less of cosmetic nature by now.  I'm  more  than  busy  doing
    other  things  actually.   If  someone  else  want's to learn
    abount  the  internals  of  rules,  touch   it   and   become
    responsible for it.

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #



Fri, 7 Jul 2000 02:43:27 +0200 (MEST)
JanWieck@t-online.de (Jan Wieck)
Re: Memo on coding practices: strcmp() does not yield bool
JanWieck@t-online.de (Jan Wieck) writes:
>     Oh  boy.  Originally  I  wrote  ruleutils.c  as  a proof that
>     rewrite rules "can" tell what the  original  rule  (or  view)
>     looked  like.  Someone  called it a "magic piece of software"
>     and we adopted it as a useful thing to dump views  and  rules
>     (what  we  wheren't  able before).  Now you blame me for it's
>     uglyness.

Hey, I didn't mean to sound like I was picking on you in particular.
There are a lot of instances of that coding practice in our system.

I just used ruleutils.c as an example because that was where the
reported bug was --- and yes, this was from a regression test porting
failure report; the rules output was missing some AS clauses it
should've had.  With this fix, we pass regress tests on MkLinux PPC 
at default optimization level.  This bug was probably masked before
because we couldn't compile with optimization on that platform,
due to the far worse portability bugs in fmgr.

The way I see it, today we learned one more tidbit about how to produce
portable C code.  You didn't know it before, and neither did I.  No
shame in that.

The more bugs we fix, the higher our standards become.  It's all
part of the process of world domination ;-)

			regards, tom lane

Thu, 06 Jul 2000 22:16:52 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Memo on coding practices: strcmp() does not yield bool 
I checked through the code and found no other problem cases.

> I've occasionally griped that I do not like the coding practice of
> writing
> 	if (strcmp(foo, bar))
> to mean
> 	if (strcmp(foo, bar) != 0)
> or its inverse
> 	if (!strcmp(foo, bar))
> to mean
> 	if (strcmp(foo, bar) == 0)
> 
> My past objection to this has been purely stylistic: it's too easy
> to read these constructs backwards, eg to think "!strcmp()" means
> "not equal".  However, I've now had my nose rubbed in the fact that
> this habit is actually dangerous.
> 
> Up till just now, ruleutils.c contained code like this:
> 
>         bool        tell_as = FALSE;
> 
>         /* Check if we must say AS ... */
>         if (!IsA(tle->expr, Var))
>             tell_as = strcmp(tle->resdom->resname, "?column?");
> 
> 	/* more code... */
> 
>         if (tell_as)
>             /* do something */
> 
> This is subtly wrong, because it will work as intended on many
> platforms.  But on some platforms, strcmp is capable of yielding
> values that are not 0 but whose low 8 bits are all 0.  Stuff that
> into a char-sized "bool" variable, and all of a sudden it's zero,
> reversing the intended behavior of the test.
> 
> Correct, portable coding of course is
> 
>             tell_as = strcmp(tle->resdom->resname, "?column?") != 0;
> 
> This error would not have happened if the author of this code had
> been in the habit of regarding strcmp's result as something to compare
> against 0, rather than as equivalent to a boolean value.  So, I assert
> that the above-mentioned coding practice is dangerous, because it can
> lead you to do things that aren't portable.
> 
> I'm not planning to engage in a wholesale search-and-destroy mission
> for misuses of strcmp and friends just at the moment, but maybe someone
> should --- we may have comparable portability bugs elsewhere.  In any
> case I suggest we avoid this coding practice in future.
> 
> 			regards, tom lane
> 


-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Thu, 6 Jul 2000 23:26:59 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Memo on coding practices: strcmp() does not yield bool
My experience has been the same as Tom's (only perhaps more so, he
didn't actually admit to being confused by "if (strcmp(...))" :)

I've always been uncomfortable with that "implicit zero", and have been
confused more than once by code which does not include the "= 0" or "!=
0". When I'm modifying code, I usually will add this stuff in.

                      - Thomas

Fri, 07 Jul 2000 06:07:01 +0000
Thomas Lockhart <lockhart@alumni.caltech.edu>
Re: Memo on coding practices: strcmp() does not yield bool
On Thu, 6 Jul 2000, Tom Lane wrote:

> I've occasionally griped that I do not like the coding practice of
> writing
> 	if (strcmp(foo, bar))
> to mean
> 	if (strcmp(foo, bar) != 0)

Why not define a macro to avoid the urge to take shortcuts like that in
the future?

#define streq(a,b) (strcmp((a), (b))==0)

This is guaranteed to yield 1 or 0, and it's very readable.


-- 
Peter Eisentraut                  Sernanders vaeg 10:115
peter_e@gmx.net                   75262 Uppsala
http://yi.org/peter-e/            Sweden


Fri, 7 Jul 2000 07:43:51 -0400 (EDT)
eisentrp@csis.gvsu.edu
Re: Memo on coding practices: strcmp() does not yield bool
Ed Loehr <eloehr@austin.rr.com> writes:
> I see your examples demonstrate the danger of inappropriate or
> inattentive type conversion (e.g., splicing an int into a char), but I'm
> missing the danger you see, beyond a style offense, of "if (strcmp(foo,
> bar))"?

"if (strcmp(foo, bar))" is portable, no doubt about it.  My point is
that the idiom encourages one to think of strcmp() as yielding bool,
which leads directly to the sort of thinko I exhibited.  It's a
slippery-slope argument, basically.

I had always disliked the idiom on stylistic grounds, but I never quite
had a rational reason why.  Now I do: it's a type violation.  If C had
a distinction between int and bool then you'd not be allowed to write
this.  As an old Pascal programmer I prefer to think of the two types
as distinct...

			regards, tom lane

Fri, 07 Jul 2000 13:05:18 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Memo on coding practices: strcmp() does not yield bool 