Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Code review requested

Reply
Thread Tools

Code review requested

 
 
ImpalerCore
Guest
Posts: n/a
 
      09-21-2012
On Sep 20, 6:48*pm, James Kuyper <jameskuy...@verizon.net> wrote:
> On 09/20/2012 05:52 PM, ImpalerCore wrote:
>
> > On Sep 20, 5:09 pm, James Kuyper <jameskuy...@verizon.net> wrote:

> ...
> >> It's not just integer types you need to worry about, (bool)0.5 and
> >> (bool)&object won't work properly, either.

>
> > I have not experienced code that did those type conversions, so I
> > didn't document it at the time. *Have you happened to experience
> > either of those type conversions in the wild?

>
> 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.
 
Reply With Quote
 
 
 
 
ImpalerCore
Guest
Posts: n/a
 
      09-21-2012
On Sep 20, 7:54*pm, Keith Thompson <ks...@mib.org> wrote:
> ImpalerCore <jadil...@gmail.com> writes:
>
> [...]> 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.
 
Reply With Quote
 
 
 
 
Jorgen Grahn
Guest
Posts: n/a
 
      09-22-2012
On Thu, 2012-09-20, Bart Vandewoestyne wrote:
> On 09/20/2012 06:02 PM, ImpalerCore wrote:
>>
>> The only thing that I'm not fond of is the typedefs used to hide the
>> pointer type. The pointer '*' symbol is an important type modifier
>> and I like to show them explicitly for easier semantic understanding
>> of a function interface.


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

....
>> types. While I can get used to most naming conventions, my preference
>> is to make the pointer attribute of a type explicit. By the same
>> token, I prefer using 'char*' explicitly over something like 'string'.


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

--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
 
Reply With Quote
 
Keith Thompson
Guest
Posts: n/a
 
      09-23-2012
David Brown <> writes:
> 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.

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
Will write code for food.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
BartC
Guest
Posts: n/a
 
      09-23-2012
"David Brown" <> wrote in message
news...

> 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.

--
Bartc

 
Reply With Quote
 
嘱 Tiib
Guest
Posts: n/a
 
      09-23-2012
On Sunday, 23 September 2012 13:50:03 UTC+3, David Brown wrote:
> 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.

 
Reply With Quote
 
Keith Thompson
Guest
Posts: n/a
 
      09-23-2012
脰枚 Tiib <> writes:
> 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".

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
Will write code for food.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
Ian Collins
Guest
Posts: n/a
 
      09-23-2012
On 09/24/12 10:39, Keith Thompson wrote:
> 嘱 Tiib<> writes:
>> 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.

--
Ian Collins
 
Reply With Quote
 
Malcolm McLean
Guest
Posts: n/a
 
      09-23-2012
讘转讗专讬讱 讬讜诐 专讗砖讜谉, 23 讘住驻讟诪讘专 2012 23:39:11 UTC+1, 诪讗转 Keith Thompson:
> 脰枚 Tiib <> writes:
>
> 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.
 
Reply With Quote
 
Ian Collins
Guest
Posts: n/a
 
      09-23-2012
On 09/24/12 11:03, Malcolm McLean wrote:
> 讘转讗专讬讱 讬讜诐 专讗砖讜谉, 23 讘住驻讟诪讘专 2012 23:39:11 UTC+1, 诪讗转 Keith Thompson:
>> 脰枚 Tiib<> writes:
>>
>> 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.


And everyone else thinks it's a MACRO!

--
Ian Collins
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
A little light code review requested luserXtrog C Programming 2 08-18-2010 04:15 AM
Re: Code review requested, Accelerated C++ Vyacheslav Kononenko C++ 0 10-18-2004 01:58 PM
Re: Code review requested, Accelerated C++ Mike Wahler C++ 1 10-18-2004 03:43 AM
Code review requested Ed Java 1 07-19-2004 12:11 PM
Code review requested Ian Pilcher Java 0 12-15-2003 04:53 AM



Advertisments
 



1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57