Thread: Memo on coding practices: strcmp() does not yield bool
I've occasionally griped that I do not like the coding practice of
writingif (strcmp(foo, bar))
to meanif (strcmp(foo, bar) != 0)
or its inverseif (!strcmp(foo, bar))
to meanif (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
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
Re: Memo on coding practices: strcmp() does not yield bool
From
JanWieck@t-online.de (Jan Wieck)
Date:
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 interpreterfor 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.cis checked in the regression suite. None of the ports reported problems so far, so the requested stylefixes 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 #
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
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, Pennsylvania19026
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
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
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