2 style questions

M

Martin Ambuhl

Sander said:
1.
I was having a discussion with somebody and I think it's just religious.
We are developing some software and he was looking through my code.
I use if (pointer) to test a pointer for NULL. He says it must be if
(p!=NULL).

"Must" is obviously wrong.
The arguments are that it's more readable

Some people make this claim, so I believe that it is true for _them_. It
is not true for me, and, I suspect, it is not true for most experienced C
programmers.
and that if (p) is not portable.

This is completely false. Do not take any further advice from anyone
making such obviously false statements.

[...]
Then I started looking through his code and what I noticed that he very
often does things like this:

char *func(char **out)
{
*out = malloc(100);
return malloc(10);
}

This might also be a style issue, but it's a style where it's very easy to
introduce bugs (and indeed I found a few)
and even more easy to introduce bugs if you don't know the internals of the
function. So, what do you think about this style?

Failure to check the return value from malloc is much worse than a "style
issue." If this failure is in supposedly professional code, it shows
incompetence. The use of hard-coded magic numbers shows a cavalier
indifference to maintenance unbecoming a professional.
 
P

Paul Hsieh

It's certainly more dangerous and I already found a mem leak in his own
code.

As a rule of thumb if you can avoid splitting your functions in such a way that
you allocate memory that is to be freed by another function, then you will have
fewer memory leak problems. However, there are tools and mechanisms that can
help you with those issues, and you have to consider the cost of alternatives:

1) If you use global/static memory then you typically end up with re-entrancy
or multithreading restrictions.
2) If you force memory allocation-usage-then freeing within each of your
functions, then you code may become inflexible. You can try to improve your
flexibility by using callbacks and automatic iteration over those callbacks in
these functions, however this kind of thing can be convoluted and it can be
complicated to maintain a sensible thread of the execution sequence.

Usually more important than the details of passing allocated memory out of
functions, is the semantics of the datastructures that are passed around. If,
for example, you only pass out completely initialized objects (which may
contain allocated sub-parts) that have been wrapped in a typedef or struct
definition, then its usually easier to simply map semantics to the object in an
intuitive way, rather that worry about what could potentially go wrong from
misuse of the details of your objects.

You can find an example of what I am talking about in: http://bstring.sf.net/ .
Its just a string library -- it allocates memory for strings that it passes
out, and there is no issue so long as at some point those strings are passed
back to the destruction function in the library.
 
R

Richard Heathfield

The said:
So you should write

if ((p != NULL) == !TRUE)

That doesn't follow. For a start, TRUE is undeclared. Secondly, if you
declared it and then gave it a non-pathological value - for a name like
TRUE I'd expect it to be non-zero - you would find that you have reversed
the sense of the test and introduced a bug. Thirdly, it is less readable
than the simple if(p != NULL), IMHO.
assert() is ALWAYS a source of confusing the user and destroying data
consistency!

No, it isn't.
assert is NOT designed to test data!

I never said otherwise. By all means search the archives for evidence of my
position on assert.
 
T

The Real OS/2 Guy

Others have explained that "if (p)" and "if (p != NULL)" are functionally
equivalent. I prefer to write if "(p != NULL)" because in my situation I
cannot assume that everyone who reads the code is an experienced C
prorgrammer.

Hm, an unexperient C programmer has to learn it anyway. So when h/sh/e
fails over that ist time for him to learn more about C.

At least not an argument.

I myself have seldom been paid directly to produce C code.
Usually the code is incidental to something else, or written just for fun.

When I am reading code, if(p) doesn't bother me in the least. The only
problem would be if it were in a context where it was difficult to find
out what type p is. This is an unlikely scenario, and anyway, most
programming oriented editors let you jump right to the declaration.

Dosn't mater what type p is. When one had used 'p' for thome type
other as a pointer you would fall about that some point earlier and
whine about the abstuse naming the programmer was using - even as the
name p is allwowed for any data type the implicit thinking of an
human programmer would assign that name to an pointer type, not
struct, int or other.

in an conditional expression any C programmer would assign an single
variable name with

C syntax human reads based on code content
-------- -----------
if (x) is x TRUE?
if (!x) is x false?
if (x) is x 0/0x00/000/zero
while (p) while pointer is valid?
for (; y; ) while y is true?
for (; y; ) has y reached 0?
for (; !p; ) until pointer is valid
......

as s/h/e knows that C makes an implicite compare to (type) 0 it's more
easy to
read c = p ? *p++ : 0 as 'when p is valid then assign content of that
p points to to c and increment the pointer, else assign 0 to c' as
when 'p is NULL then pointer is valid and ....'

Comparing explicite to (type)0 is done only by very unexperienced C
programmers (and only if they have learned another programming
language prior).

I'm not sure what you mean.

A special case to hide malloc from to be visible, to make code more
complicated by using wrappers around malloc, to make maintenance more
complicate.
 
M

Malcolm

Jens Prüfer said:
Now I am only an occasional C programmer but I have
learned to put the constant first in comparisons, because especially
when testing for "equals", it makes the compiler complain when you are
using "=" instead of "==". Example:

if (CONST == a) ...

and not

if (a == CONST) ...
It is a pain that if(x = 100) happens to be a legal C construct. Not all
compilers warn about it.
However if(100 = x) is much more difficult to read - like spelling a work
backwards the effect of one such line is small, but the cumulative effect of
many such features is to make reading code very difficult.
if(CONST == x) lulls you into a false sense of security, because there can
be no protection from if(N = i) (N and i both variables).
 
K

Kevin Goodsell

Malcolm said:
It is a pain that if(x = 100) happens to be a legal C construct. Not all
compilers warn about it.
However if(100 = x) is much more difficult to read - like spelling a work
backwards the effect of one such line is small, but the cumulative effect of
many such features is to make reading code very difficult.

I wouldn't say it's "much more difficult." I haven't personally adopted
the technique, mainly because I don't really like the way it looks and
reads, but I think I could get used to it very quickly if I did start
using it (which I've been considering doing). The thing is, /any/ C
construct is difficult to read if you aren't accustomed to it, and in
this particular case it only requires reading from right to left instead
of left to right. C programmers are already used to reading that
direction (or from the inside out) in declarations. So I don't accept
readability is a major argument against this technique.
if(CONST == x) lulls you into a false sense of security, because there can
be no protection from if(N = i) (N and i both variables).

Yes, it is one of many C "quick fixes" that don't solve the real
problem, but do catch some instances (like NULLing freed pointers).

-Kevin
 
K

Kevin Goodsell

Sander said:
1.
I was having a discussion with somebody and I think it's just religious.
We are developing some software and he was looking through my code.
I use if (pointer) to test a pointer for NULL. He says it must be if
(p!=NULL).

Maybe I'm weird, but I kind of think that

if (p)

is (slightly) easier to read than

if (p != NULL)

But I also think that

if (p == NULL)

is (significantly) easier to read than

if (!p)

I guess this is because 'if (p)' and 'if (p == NULL)' avoid negatives.
Negations (!, !=) tend to be confusing to me - expressions with fewer
negations are usually much easier to understand.

Does anyone else feel this way? Or am I just crazy?

-Kevin
 
R

Richard Heathfield

Malcolm said:
However if(100 = x) is much more difficult to read

Well, the compiler will have no problem reading it on your behalf and
drawing your attention to the error.
- like spelling a work
backwards the effect of one such line is small, but the cumulative effect
of many such features is to make reading code very difficult.

I have not found this to be the case. Nevertheless, you are in good company,
since Dan Pop agrees with you 100% on this issue. I'm sure that will be of
some comfort to you. ;-)
if(CONST == x) lulls you into a false sense of security, because there can
be no protection from if(N = i) (N and i both variables).

No, it doesn't lull any sensible programmer into a false sense of security.
const==var is not a silver bullet, and nobody with any sense would pretend
otherwise. It's just a useful device, not a magic wand.
 
C

CBFalconer

Kevin said:
Sander wrote:
.... snip ...

But I also think that

if (p == NULL)

is (significantly) easier to read than

if (!p)

I guess this is because 'if (p)' and 'if (p == NULL)' avoid negatives.
Negations (!, !=) tend to be confusing to me - expressions with fewer
negations are usually much easier to understand.

Another possibility:

#include <iso646.h>
.....
if (not p)
 
P

Paul Hsieh

1.
I was having a discussion with somebody and I think it's just religious.
We are developing some software and he was looking through my code.
I use if (pointer) to test a pointer for NULL. He says it must be if
(p!=NULL).

They are identical. The issue is not which one you choose, but rather that it
matters at all. If either of you have a problem with one method over the other
then its that person's problem. And its a serious one -- if a person is unable
to see a trivial equivalence in their mind, then its indicative of an improper
focus or otherwise very low mental ability.

One has religious arguments about religion, not technical equivalents. The
only merit of caring is that leaving off the != NULL has fewer characters, and
thus occupies less screen space -- but its horizontal screen space which
rarely matters.
The arguments are that it's more readable and that if (p) is not portable.

"if (p)" is portable, and since it has an equivalent meaning and can't really
be mistaken to mean something else, it can't be less readable.
I said the first is a style issue [...]

Its a non-issue. They are the same thing. You lose IQ points for every second
you waste talking to your codeveloper about it. You don't have more important
issues to talk about?
2.
Then I started looking through his code and what I noticed that he very
often does things like this:

char *func(char **out)
{
*out = malloc(100);
return malloc(10);
}

This might also be a style issue, but it's a style where it's very easy to
introduce bugs (and indeed I found a few)
and even more easy to introduce bugs if you don't know the internals of the
function. So, what do you think about this style?

For me, the value of encapsulating blocks of code into functions is for the
functions to take responsibility for credibly translating input parameters to
output parameters. If a function, such as this, is essentially perform two
side-effects, it had better be worth while to do so.

I would be very suspicious of the code above, because both mallocs are not
stored in the same structure. Hence this inherently assumes that some higher
level logic is going to tie those two mallocs together which means that "func"
is not taking responsibility for tying them together. There are also 3
possible error conditions (each malloc failing, or both of them failing) all of
which have to be determined by higher level code. For the same semantics, I
would rewrite the above as:

int func (char ** out1, char ** out2) {
if (out1) *out1 = malloc (100);
if (out2) *out2 = malloc (10);
return (out1 == NULL || *out1 != NULL)
&& (out2 == NULL || *out2 != NULL);
}

In this way its more obvious that there are two outputs (since neither
parameter is decorated as "const") and the return value will tell you whether
or not there was any failure at all. Also the effect of passing in a NULL
pointer will have the effect of deactivating that output rather than simply
leading to undefined behaviour.

For someone like me, its hard to justify writing a function like the above,
however. The reason for having two mallocs *must* be because out1 and out2 are
somehow tied together possibly in the same object. Also if these are really
"char *"'s why don't you also at least initialize them with '\0's in them?
Passing around bare mallocs (i.e., where the contents are indeterminant) is
generally kind of useless.
 
T

The Real OS/2 Guy

That doesn't follow. For a start, TRUE is undeclared. Secondly, if you
declared it and then gave it a non-pathological value - for a name like
TRUE I'd expect it to be non-zero - you would find that you have reversed
the sense of the test and introduced a bug. Thirdly, it is less readable
than the simple if(p != NULL), IMHO.

Hm, rally? #define TRUE to something that is NOT 0. Now tell me how
!TRUE can be evaluated to something else than 0.
No, it isn't.

A user who is not a programmer gets a shock, a live crisis if he gets
a screen saying 'core dump' or simply 'trap' or when his GUI program
ends commentless......

assert() is designed to break the program at the point it is called.
This confuses any user - even as it helps any developer to catch
errors s/h/e or her/his colleauge has made.
I never said otherwise. By all means search the archives for evidence of my
position on assert.
Newsgroups are not to communicate privately. If I had mean you in
special I had sayed so.
 
N

nrk

The said:
Hm, rally? #define TRUE to something that is NOT 0. Now tell me how
!TRUE can be evaluated to something else than 0.

You are missing Richard's point. If TRUE is non-zero, then your test is the
exact opposite of what Richard was testing using:
if ( p != NULL )

If TRUE is zero, your test is obfuscated and way less readable than
Richard's.

All this just goes to show that Richard's style preference is in no way
comparable to your absurdity, and that your arguments against it are
meritless. FWIW, my own preference doesn't match Richard's. I prefer a
simple if ( p ), as it saves a few keystrokes without reducing readability
IMO.

To belabor the obvious:
/* if p is not NULL */
if ( p != NULL )

/* For TRUE == 1, this is the same as above
and probably what you intended to say */
if ( (p != NULL) == TRUE )

/* For TRUE != 0, this is the negation of above */
if ( (p != NULL) == !TRUE )
/* or if ( !( p != NULL) ) or if ( p == NULL ) */

-nrk.

<snip>
 
C

Christopher Benson-Manica

Sander said:
Ai, I was not very clear about the second.
It's not about returning two values, it's about returning dynamic memory
from a function.
I consider this very bad style, to the point that it's really crappy
unexperienced C coding.

It has to have *some* value, otherwise why would *so* many people
write and use the ubiquitous (but nonstandard!) strdup()?
 
R

Richard Heathfield

nrk said:
You are missing Richard's point. If TRUE is non-zero, then your test is
the exact opposite of what Richard was testing using:
if ( p != NULL )

If TRUE is zero, your test is obfuscated and way less readable than
Richard's.

All this just goes to show that Richard's style preference is in no way
comparable to your absurdity, and that your arguments against it are
meritless.

I couldn't have put it better myself.
FWIW, my own preference doesn't match Richard's. I prefer a
simple if ( p ), as it saves a few keystrokes without reducing readability
IMO.

I just wanted to add that I don't think if(p) is unreadable. I just happen
to prefer to reserve if(foo) for testing the values of objects whose
purpose is clearly Boolean in nature. Examples include:

if(!done)
if(isalpha((unsigned char)ch))
if(PrintSpoolIsFull)

I know I'm not unique in having this preference, but on the other hand I'm
not silly enough to think I can persuade the whole world to fall into line,
so I'm perfectly content to read code that uses your preference.
 
E

E. Robert Tisdale

nrk said:
I prefer a simple

if (p)

as it saves a few keystrokes without reducing readability IMO.

It implies an implicit conversion

if ((bool)p)

(or

if ((int)p)

for Olde Tyme C programmers).

I prefer

if(NULL != p)

or

if (!(NULL == p))

because the result of the comparison is the appropriate type.
 
S

Sander

Paul Hsieh said:
int func (char ** out1, char ** out2) {
if (out1) *out1 = malloc (100);
if (out2) *out2 = malloc (10);
return (out1 == NULL || *out1 != NULL)
&& (out2 == NULL || *out2 != NULL);
}

or
int func(char **out1, char **out2)
{
if (out1) *out1 = malloc(100);
if (out2) *out2 = malloc(10);
return (!out1 || *out1) && (!out2 || *out2);
}

:)
 
J

John Bode

Sander said:
Ai, I was not very clear about the second.
It's not about returning two values, it's about returning dynamic memory
from a function.

Nothing wrong with that. I generally abstract out allocation and
deallocation of resources, for example:

#include <stdio.h>

typedef ... thing;

thing *new_thing (void)
{
thing *tmp = malloc (sizeof *tmp);
if (!tmp)
{
/* report out of memory */
}
else
{
/* initialize tmp to default values */
}

return tmp;
}

void destroy_thing (thing *t)
{
if (t)
{
/* if any of t's components were also */
/* allocated dynamically, release them */
/* first */
free (t);
}
}

int main (void)
{
thing *something;

something = new_thing ();
do_something_with (something);
destroy_thing (something);

return 0;
}
Sometime he does something like this:
char *fun(void) { return malloc(10); }

If you're going to take the time and trouble to wrap a function around
malloc(), one would hope that you also add some logic to detect and
report memory allocation failures. Otherwise you might as well just
call malloc() directly.
and sometimes this:
void fun(char **s) { *s = malloc(10); }

I consider this very bad style, to the point that it's really crappy
unexperienced C coding.

There's nothing wrong with the second form (although I'd check to see
if malloc() succeeded and return a status value).

Don't knock abstraction. Your coworker may not necessarily know what
he's doing, but wrapping primitive library calls in higher-level
abstract functions is hardly bad practice.
 
F

Flash Gordon

Hm, rally? #define TRUE to something that is NOT 0. Now tell me how
!TRUE can be evaluated to something else than 0.

So what you've changed it to is
if ((p != NULL) == 0)
this is different from
if (p != NULL)
because the above is the same as
if ((p != NULL) == 1)
I think you meant
if ((p != NULL) != !TRUE)
which is introducing more negations and hence making it less readable.

My personal preference is
if (p != NULL)
or for integers used as integers
if (i != 0)
or for integers used as booleans/flags
if (b)
A user who is not a programmer gets a shock, a live crisis if he gets
a screen saying 'core dump' or simply 'trap' or when his GUI program
ends commentless......

assert() is designed to break the program at the point it is called.
This confuses any user - even as it helps any developer to catch
errors s/h/e or her/his colleauge has made.

Depends. Users of the application I use would be less surprised (and
concerned by) an assert failure than by the application providing
incorrect results or corrupting data, and to this end I have put in
some error checking (not asserts) that aborts the program if it detects
some types of internal errors. Of course, what they really want is for
there to be no errors of any form.
 
F

Flash Gordon

Another possibility:

#include <iso646.h>
....
if (not p)

This I would not go for because it limits portability to implementations
implementing ISO646, but I would not have any difficulty reading it. I
find
if (p)
if (!p)
if (p==NULL)
if (p!=NULL)
all equally legible. However, I prefer explicit comparison with NULL
when testing a pointer and explicit comparison with 0 when testing
a variable used as an integer (as opposed to a variable used as a
boolean flag). However, I don't feel strongly enough about it to request
others on the projects I am involved in use the same conventions.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
474,125
Messages
2,570,748
Members
47,302
Latest member
MitziWragg

Latest Threads

Top