Code review requested

[ImpalerCore]

My project is still governed by a 1997 agreement with our client which
requires us to write code which "conforms" to C90 plus the two Technical
Corrigenda. Technically, that's not exactly a restrictive requirement -
"War and Peace" qualifies as conforming C code - but I try to follow the
spirit of the requirement, even if it was incorrectly worded.

I long ago adopted a policy of avoiding defining variables whose sole
job is to keep track of whether or not a condition is true; I prefer
re-stating the condition - it often leads to clearer code, and generally
does the right thing when the condition involves variables whose values
have changed since the last time it was evaluated. If the condition does
not actually need to be re-evaluated, most modern compilers can be
counted on to detect that fact (more reliably than I can), and use an
unnamed temporary for the same purpose that a named boolean variable
would have been used for. Therefore, I think this is a feature I won't
make much use of, even when I can do so.

While I think that is a valid rule of thumb, there are cases where
caching the result in a properly named variable can lead to more
readable and better performing code. In longer pieces of code, like
when parsing format strings, using booleans to represent state
conditions detected when traversing the string can be much easier to
read than repeating terse conditions. For example, if I need to
detect a leading zero, I'm perfectly happy to use a 'bool' variable to
name and track the condition.

\pseudocode
bool is_valid_format_X( const char* s )
{
bool leading_zero = false;

while ( ... )
{
...

if ( *p == '0' ) {
leading_zero = true;
}

...

if ( leading_zero ) {
...
}

++p;
}

...

if ( leading_zero ) {
...
}

return true;
}
\endcode

But the real advantage of defining 'bool' in C90 is that it is a
superior type name for 'int' when defining function interfaces that
take boolean parameters or that have boolean return values.

Here is an example that identifies a valid gregorian date from my date
library.

\code
bool c_is_valid_greg_date( struct c_greg_date date )
{
return ( ( date.year == INT16_MAX ) &&
( date.month == INT8_MAX ) && ( date.day == INT8_MAX ) )||
( ( date.year == INT16_MIN ) &&
( date.month == INT8_MIN ) && ( date.day == INT8_MIN ) )||
( ( date.year >= C_GREGORIAN_EPOCH_YEAR ) &&
( date.month >= C_JANUARY ) &&
( date.month <= C_DECEMBER ) &&
( date.day > 0 ) &&
( date.day <= gc_days_in_month_table[LEAP_INDEX(date.year)]
[date.month - 1] ) );
}
\endcode

If the 'bool' type is replaced with 'int', as in 'int
c_is_valid_greg_date( struct c_greg_date date )', now I need to
consider the possibility of whether the range of values is something
other than '1', or '0'. Functionally it would still operate as
intended. But from a cognitive perspective, 'bool' implies a narrower
semantic meaning to the return type than 'int' does. Do you use 'int'
to represent all your boolean parameters and return types? I would
find it more cognitively difficult to separate 'int' parameters that
are truly integers, and those that are booleans in disguise.

\code
/*!
* \brief Search a \c c_list for an object that satisfies the
* property evaluated by the predicate function \c pred_fn.
* \param list A \c c_list.
* \param property A property related to a list object.
* \param pred_fn The predicate function.
* \return The list node of the first matching object, or \c NULL if
* not found.
*/
struct c_list* c_list_search( struct c_list* list,
const void* property,
bool (*pred_fn)( const void*, const
void* ) );

/*!
* \brief Insert a new object into the \c c_list in sorted order.
* \param list A \c c_list.
* \param object The object.
* \param cmp_fn The comparison function.
* \return The head of the \c c_list.
*/
struct c_list*
c_list_insert_sorted( struct c_list* list,
void* object,
int (*cmp_fn)( const void*, const void* ) );
\endcode

In this example, 'bool' more clearly indicates the kind of function I
expect when searching a list verses sorting a list. For example,
searching a list of strings for strings that match a file suffix does
not require the same return type that a lexicographical comparison
would need. The comparison function mandates an 'int' return type for
to sort a list in alphabetic order, the predicate function for
searching the list does not. The 'bool' typedef clearly identifies
the return type expected to define your own search function to use
these list interface functions.

In the scenario of converting a floating point to bool using
'(bool)0.5', you can implement coding standards that prohibit explicit
or implicit conversions of floating point values to bool, which I tend
to avoid in general. The only expressions I use in 'if' statements
with implied equality are booleans and pointers. If I want to check
whether a floating point value is equal to zero, I make a point to
make that comparison explicit '== 0.'.

However, if I ever do use _Bool, implicit conversions from pointer
values to _Bool stand a very good chance of being involved. I often
write if(p) rather than if(p!=NULL), which is essentially the same idea..

I guess I don't see the issue if you avoid 'if ( (bool)p )' in your
expressions, and I think the only issue would be for systems where
NULL is not equivalent to 'false' (please correct me if I'm missing
something). And a typedef of 'enum { false, true }' to 'bool' doesn't
change the semantic meaning of either 'if (p)' or 'if (p != NULL)' in
C90 or C99 unless you do something like 'if ( (bool)p )', which is not
idiomatic C in either standards.

Feel free not to use it, but I find it extremely valuable in defining
semantics for function interfaces. Would I argue for it if I was
working on your project? Probably as I think the cognitive benefits
outweigh the risks of what you'd write in idiomatic C code. And I've
found the typedef to be compatible in C90 environments with judicious
use.

Best regards,
John D.

 

[ImpalerCore]

[...]> I do not particularly like the way you use typedefs to alias struct

types.  I prefer to typedef a struct type to the same name (as in
'typedef struct c_list c_list;'), or to place the trailing suffix in
the struct identifier.

struct Table_;
typedef struct Table_ Table;

[...]

Yes, if you're going to use a typedef for a struct type, there's no need
to use a different identifier than the struct tag (likewise for unions
and enums).

I definitely agree on using the exact same name for struct and typedef
types when creating aliases for 'struct type'. I don't see a viable
reason where one would want to have 'struct type' and 'typedef
something_else type' to refer to two logically distinct types; there's
the risk that it could be quite confusing to for other developers to
read.

My own preference is not to use a typedef at all, unless I want to hide
the fact that the type is a struct.  For example, I'd just write:

    struct table {
        /* member declarations */
    };

and then refer to the type as "struct table".  I don't see much
point in defining a second name (like "table") for a type that
already has a perfectly good name ("struct table").  A lot of
people like the advantage of not having to type the extra word,
which is a valid argument, but not one that I find convincing.

In C++ where objects are defined in classes, it would be considered
superfluous if one needed to use a 'class Doodle variable;' to each
declaration. Since I learned C++ before C, that has certainly
affected my lack of inhibition of using 'type variable;' rather than
'struct type variable;', since I was used to 'class' "polluting" the
global namespace in C++.

However, I like to use 'struct type' in C for function interfaces (it
gives it a more formal feeling as in you know that it's not a typedef
for something else if 'struct' is explicit, what you see is what you
get), and use type aliases in example or application code. If one
wants to maintain formatted code within a fixed number of columns (I
aim for 70 with an 80-column width), requiring 'struct' when passing
types to macros can make managing screen space a bit more cumbersome.

Best regards,
John D.

 

[Jorgen Grahn]

And you cannot make the important distinction between
'const struct Foo*' and plain 'struct Foo*'.

....
And there it's very visible. I use const char* much more often than
plain char*.

I can agree with this, but I'm trying to follow the book's code and
conventions, so I guess I'll stick with it for now.

It suggests a weakness in the book, though (unless it creates two
typedefs for each pointer type). No doubt people still do that today,
but to me it indicates the author learned the language before const
was invented in the mid-1980s.

/Jorgen

 

[Keith Thompson]

On 21/09/2012 01:54, Keith Thompson wrote: [...]

My own preference is not to use a typedef at all, unless I want to hide
the fact that the type is a struct. For example, I'd just write:

struct table {
/* member declarations */
};

and then refer to the type as "struct table". I don't see much
point in defining a second name (like "table") for a type that
already has a perfectly good name ("struct table"). A lot of
people like the advantage of not having to type the extra word,
which is a valid argument, but not one that I find convincing.

My own preference is the opposite - I don't see much point in having to
call the thing "struct table" when you can just call it "table".

[snip]

Also a valid preference.

 

[BartC]

My own preference is the opposite - I don't see much point in having to
call the thing "struct table" when you can just call it "table". Why do
you have to know that a "table" happens to be a "struct"?

It allows you do this:

typedef struct A{ ...}T;
typedef int A;

struct A x;
A y;

Ie. it allows the struct name to be used for other things Or you can do
this:

typedef struct A{ ...}T;

struct A A;

Or this:

typedef struct A{ ...}A;

struct A x;
A y;

where both x and y are of the same type.

But you get the idea -- I agree with you! This extra namespace of structs is
just a source of confusion. I think structs should be anonymous.

 

[Öö Tiib]

Why do
you have to know that a "table" happens to be a "struct"? Either you
are going to use the contents of a "table" - in which case you need to
know lots of detail about it, so "struct" is a waste of time - or you
are going to use it blindly (via pointers, memcpy with sizeof, etc.), in
which case it doesn't matter if it is a struct or a POD (in C++
terminology).

Yes, either you know well what it is or it is given to you as a handle.

I almost invariably define structure types as:

typedef struct { ... } table;

The only time I would write "struct table;" would be to define a forward
declaration for lists, trees, etc. And then I would later write

"typedef struct table table;"

The additinal bonus when you do that is that C++ does exactly the same typedef
implicitly. C++ compiler has to compile if it is said out explicitly
as well. As result you can use the definition in both languages:

typedef struct table { ... } table;

No additional preprocessor magic is needed.

 

[Keith Thompson]

The additinal bonus when you do that is that C++ does exactly the same typedef
implicitly. C++ compiler has to compile if it is said out explicitly
as well. As result you can use the definition in both languages:

typedef struct table { ... } table;

No additional preprocessor magic is needed.

Personally, I wouldn't bother with the typedef either in C or in C++.
In C I'd refer to the type as "struct table"; in C++, I'd refer to
it as "table".

 

[Ian Collins]

Personally, I wouldn't bother with the typedef either in C or in C++.
In C I'd refer to the type as "struct table"; in C++, I'd refer to
it as "table".

Why? It looks like you are making extra work for your self for no good
reason.

 

[Malcolm McLean]

בת×ריך ×™×•× ×¨×שון, 23 בספטמבר 2012 23:39:11 UTC+1, מ×ת Keith Thompson:

Personally, I wouldn't bother with the typedef either in C or in C++.
In C I'd refer to the type as "struct table"; in C++, I'd refer to
it as "table".

I typedef as TABLE. Then it shouts, IMBIG.

 

[Ian Collins]

בת×ריך ×™×•× ×¨×שון, 23 בספטמבר 2012 23:39:11 UTC+1, מ×ת Keith Thompson:
I typedef as TABLE. Then it shouts, IMBIG.

And everyone else thinks it's a MACRO!

 

[Keith Thompson]

On Sunday, 23 September 2012 13:50:03 UTC+3, David Brown wrote: [...]
The only time I would write "struct table;" would be to define a forward
declaration for lists, trees, etc. And then I would later write

"typedef struct table table;"

The additinal bonus when you do that is that C++ does exactly the
same typedef implicitly. C++ compiler has to compile if it is said
out explicitly as well. As result you can use the definition in both
languages:

typedef struct table { ... } table;

No additional preprocessor magic is needed.

Personally, I wouldn't bother with the typedef either in C or in C++.
In C I'd refer to the type as "struct table"; in C++, I'd refer to
it as "table".

Why? It looks like you are making extra work for your self for no
good reason.

Typing "struct" every now and then isn't that much extra work
-- and it avoids the extra work of defining a typedef. And I
don't consider it to be for no reason; it makes the code clearer.
It's not a huge deal; I don't *object* to typedefs for structs.
I just consider them unnecessary.

Note that FILE is an example of a typedef that I do consider to be
appropriate; client code shouldn't depend on the fact that it's a struct
(assuming that it even is one).

 

[Ian Collins]

Typing "struct" every now and then isn't that much extra work
-- and it avoids the extra work of defining a typedef. And I
don't consider it to be for no reason; it makes the code clearer.
It's not a huge deal; I don't *object* to typedefs for structs.
I just consider them unnecessary.

That's the bit I don't get, if it makes C code clearer (which I doubt),
why doesn't it make C++ clearer?

Note that FILE is an example of a typedef that I do consider to be
appropriate; client code shouldn't depend on the fact that it's a struct
(assuming that it even is one).

The way I see it, functions have to handle two types of struts:

opaque types (such as FILE) where the programmer has no interest in the
type.

normal struct types, where the programmer wishes to access the members.

In the first case, the typedef is required and in the second it is clear
from the use that the type is a struct. So I don't see how the struct
keyword improves clarity.

 

[Keith Thompson]

That's the bit I don't get, if it makes C code clearer (which I
doubt), why doesn't it make C++ clearer?

In C++, if I define a type as struct foo { ... }, that type's name is
"foo". In C, the type's name is "struct foo". Different languages,
different rules. In either case, I don't see much advantage in using
"typedef" to create another name for a type that already has one.

All things considered, I think I prefer the way C++ does this, but

The way I see it, functions have to handle two types of struts:

opaque types (such as FILE) where the programmer has no interest in the
type.

normal struct types, where the programmer wishes to access the members.

In the first case, the typedef is required and in the second it is clear
from the use that the type is a struct. So I don't see how the struct
keyword improves clarity.

I see a typedef as extra effort to hide the fact that the type
is a struct. And the plethora of different conventions (or lack
thereof) about the relationship between the struct tag and the
typedef name can cause confusion (though that can be avoided by
consistenly using the same identifier for both).

The case against typedefs for struct types isn't as strong as the
case against typedefs for pointer types, but they're similar.

 

[Nick Keighley]

<snip>

comments about style and layout are a bit of a waste of breath

The only thing that I'm not fond of is the typedefs used to hide the
pointer type.

yes. Absoloutly. Hiding pointer types is a bad idea.

On a different note, I prefer to place the asterisk '*' next to the
type rather than next to the variable, as in 'struct table*
my_table'.  I view the pointer modification as a 'type' modifier, not
a 'variable' modifier as C's syntax suggests.  It's just a personal
quirk that's non-standard, so feel free to ignore it.

I've always coded C as

Type *t;

and it's a pretty common style

(I may do different things for C++)

I do not particularly like the way you use typedefs to alias struct
types.

I do! I think having to explicitly use the struct keyword in
definitions is awful

struct S s;

C++ does it better.

S s;

 I prefer to typedef a struct type to the same name (as in
'typedef struct c_list c_list;'), or to place the trailing suffix in
the struct identifier.

struct Table_;
typedef struct Table_ Table;

<snip>

 

[BartC]

I see a typedef as extra effort to hide the fact that the type
is a struct.

That's what typedef does. As soon as you use it in any situation, for any
reason, then you are hiding information. Perhaps that information will be
coded in the name that typedef defines; perhaps it will hinted at in the
variables that are defined with it, or perhaps not.

And the plethora of different conventions (or lack

thereof) about the relationship between the struct tag and the
typedef name can cause confusion (though that can be avoided by
consistenly using the same identifier for both).

The case against typedefs for struct types isn't as strong as the
case against typedefs for pointer types, but they're similar.

Although structs can be defined casually, for one-time use, usually they
will be formally defined in one place. In that case it's easy to wrap a
typedef around it. It needs a name, so give it a typedef name instead of a
struct tag name (which for a lot people are confusing anyway).

Pointers don't contain a list of members and types of members that need
defining. A pointer can be defined anywhere. And an int* pointer defined in
one place has a compatible type with an int* pointer anywhere else. So there
is no reason for typedefs to enter into it.

(Except that a pointer typedef could help avoid the error where you type
int* a,b,c instead of int *a,*b,*c. I'm writing a bit of C at the minute and
this dilemma constantly crops up: does the "*" belong with the type, or with
the identifier being declared?

In function name and parameter declarations, the "*" is in the middle, so
can be either! In casts, it's with the type. In lists of declared
identifiers, it must with the identifier for second and subsequent
identifiers.

And where the "*" is in the middle, is it next to the type, with a space
before the name, or next to the name, with a space after the type? Or
exactly in the middle?!

If the syntax just said it's belongs to the type, that would make things
much simpler. It's tempting to use typedefs to eliminate the problem...)

 

[BartC]

On 24/09/2012 11:47, BartC wrote:

The line "int* a, b, c" - or "int *a, b, c" - belongs in the dustbin.

What do you mean?

Certainly if the intention was to declare 3 pointers, then it's incorrect.
If one pointer and two ints are needed, then it's fine!

If you mean that you should declare any series of variables, even if they
all share the same type, on individual lines, that that's a style issue. But
typical code I've seen seems to like declaring such variables together,
especially if related.

Which can help you appreciate more readily that they are in fact related
(even if just by type), saves clutter by not repeating the type over and
over again, and avoids wasting vertical space so that you can see more code
at a glance.

However it makes it harder to add individual comments, to edit, and
introduces the problem with the "*" I've mentioned.

 

[Ben Bacarisse]

Thinking that code like "int *a, b, c;" is acceptable is part of the
reason that people can talk about "industry standards of 10-20 defects
per KLOC". People who write code with a greater emphasis on obvious
correct code do not get that level of defects. (We can still make
mistakes, of course, but not that kind of mistake.)

I have a problem with remarks like this and it's mainly due to the lack
of data. There may well be good data for "10-20 defects per KLOC" (if
you have reference I'd be grateful because I don't know the literature
on this at all well) and there may well be good data for lower levels
than this when writing with "greater emphasis on obviously correct code"
(though I am more wary of this because it sounds rather vague) but I
wonder if any connection to C's declaration syntax has ever been made.

No doubt you have anecdotal evidence, but so do I: I don't recall ever
seeing such an error get past the first compile. Since one can do
arithmetic on both int and int *, it's possible to construct code that
compiles despite having one of these misplaced * errors, but I think it
would probably be rather contrived.

The last time I looked for data about coding errors and style rules
(more than 20 years ago) I couldn't find any (not that that means there
wasn't any of course!). Then, at least, people resorted to a kind of
talismanic programming in respect of style rules.

Given enough resource, it might be possible to collect some relevant
data now. Many projects will, by now, have long histories of bug fixes
in some tracking system (they were much less prevalent even 20 years
ago) so one could probably characterise bugs as being derived from
various syntactic infelicities, but I don't know of anyone who has done
this. Such an approach would miss any time spent fixing these sorts of
errors before they made it into committed code, but it would provide
some data at least.

<snip>

 

[BartC]

If you really want to declare more than one pointer on the same line, use
a typedef so that you are declaring multiple items of the same pointer
type "typedef int *pInt; pInt a, b, c;".

Yes, that was my original observation.

I have occasionally had good reason to declare more than one variable on
the same line ("int a, b, c"), but it's rare. And I cannot imagine any
sane code for which mixing types and pointers on the same line is
justifiable.

You'll have to blame C for that; it allows pointers, arrays of different
lengths and assorted combinations in the same declaration list! Other
languages --- the ones not blindly following C syntax -- tend not to do
that; every name in such a list is generally of exactly the same type. Saves
duplication too.

Using a typedef for a particular combination is one way of emulating that
behaviour, and avoiding one class of error. Other than always having
separate declarations of course. But do you really want to see blocks like
this in practically every function:

int i;
int j;
int k;

(Actually I do use this style - when I'm generating C code. But then no-one
has to suffer looking at it!)

 

[Rui Maciel]

No, it is not "fine". It compiles correctly, and works correctly - but
it is still not "fine".

If you are only stating your personal opinion on a style issue then it isn't
reasonable to make assertions on what a specific opinion is or is not
"fine". It's just a personal opinion on a style issue.

Yes, it's a style issue.

Programs should be written in a way that emphasises clarity and
maintainability, and minimises the risks of mistakes that are easy to
make and hard to spot. C allows for an enormous variety in the code,
and there are many classes of programming error that are syntactically
correct to the compiler, yet do not match the programmer's intentions.
(C is far from alone in this - but it does less hand-holding than many
other languages.) There are two ways to reduce the likelihood and
impact of such errors - automated tools (such as enabling lots of
warnings on the compiler), and strict style rules.

No serious style guide would ever allow "int *a, b, c;" as acceptable
code. Most would allow "int a, b, c", though some disallow that too.

Clarity is only in the eyes of the beholder. What may be clear to someone
else may not be clear to you, and vice-versa. It's something which depends
largely on the personal experience and therefore is subjective.

Yet, there are some issues which are not about clarity, but about
competence. Declaring variables is something which is taught right at the
start of a introduction to C course, and the difference between "int *a, b;"
and "int *a, *b;" is well covered in any C course. If someone has trouble
understanding that "int *a, b;" is not the same thing as "int *a, *b;" then
we aren't dealing with a style issue, and complaining about which style has
been adopted does nothing to tackle or fix the real problem.

Thinking that code like "int *a, b, c;" is acceptable is part of the
reason that people can talk about "industry standards of 10-20 defects
per KLOC".

Nonsense. You may believe that, if you were forced to declare variables in
that fashion, you would write code with up to 20 defects/KLOC, but that
doesn't mean that "int *a, b, c;" is the bane of the computer world.

People who write code with a greater emphasis on obvious
correct code do not get that level of defects. (We can still make
mistakes, of course, but not that kind of mistake.)

If you really want to declare more than one pointer on the same line,
use a typedef so that you are declaring multiple items of the same
pointer type "typedef int *pInt; pInt a, b, c;".

Here is another example on how clarity is only in the eyes of the beholder.
In this case, sprinkling gratuitous alternative type definitions for no
reason other than to declare a pointer to an object in a certain fashion
only ends up adding an unnecessary layer of obscurity that doesn't simplify
anything, and only makes code harder to read and to follow. Who will be
able tell just by looking at it if something like "a = &b;" isn't a bug, if
'a' is of type Foo and 'b' is of type Bar? If the declaration was actually
"int *a, b;" then it would be as clear and straight-forward as it could get.

Get a bigger screen, or use fewer variables. And declare your variables
as you first use them (C99/C++ style) rather than putting a whole pile
at the start of the function.

As an alternative, one could simply try to learn how to declare variables in
C.

I have occasionally had good reason to declare more than one variable on
the same line ("int a, b, c"), but it's rare. And I cannot imagine any
sane code for which mixing types and pointers on the same line is
justifiable.

That's fine, but you should understand that your decision is only based on
your personal opinion on a style issue. It doesn't mean that your coding
style is actually sane, let alone that any difference in coding styles would
lead to insane code.


Rui Maciel

 

[Nick Keighley]

On 24/09/2012 09:26, Keith Thompson wrote:


(This is getting a bit off-topic, but I'll ask anyway.)

Why would you have a case against typedefs for pointer types?  You seem
to have a general dislike of typedefs!

I find pointer typedefs are often useful - and in some cases they are
essential.

I've never found them essential.

 I don't meant they are essential to writing code that will
compile - but they are sometimes essential to writing code that is clear
and unambiguous to the reader and the writer.

In my work, const and volatile qualifiers are common, so it is not
uncommon for the type of a pointer to contain many parts.  This makes it
hard to read, and easy to get wrong:

I don't work in an environment where volatile is common so i may not
face the same problems as you

        uint8_t volatile * const p = ...;

Is that a constant pointer to volatile data, or a volatile pointer to
constant data, or a pointer to a volatile constant?

typedef volatile uint8_t *pVolUint8_t;
const pVolUint8_t p = ...;
bletch

Here there is no doubt what is volatile, and what is const, and how the
parts are related.

I seldom have more than two "parts" to a type (either in usage, or as
part of a typedef), and never more than three - typedefs let you put it
together step by clear and unambiguous step.

Typedefs are even more important if you are talking about function pointers.

I'll agree typedefs are necessary for function pointers

Naming conventions are also very useful here - all my pointer types
being with "p" (or "P" if you prefer), and all function pointer types
begin with "f" (or "F" if you prefer).

ug. Hungarian.