From: <rockbox_at_diffenbach.org>
Date: Sat, 1 Feb 2003 18:43:47 -0500

Thanks for your critique.

I'll switch to C Style comments.

As far as the tabs/spaces and end of lines, I'm conforming in my editor (I
think); I'm sure the email software changes the EOLs.

I'll break at 79 columns.

And you're right about packing the struct if I want to use it as an array of
bytes; I assumed 4 byte alignment and I should not have done so.

As far as the typedefs, I agree that having a myriad of typedefs (as in MS
Windows headers) can obscure the code. But I think that typedefs are useful
for making explicit programmer intent: when a (C or C++) programmer writes
"long" he means the compiler's (or the machine's) long, whatever size that
is; but by" dword" he means 4 bytes regardless of the compiler/OS being run
on; the typedef exists to describe how that type is built out of compiler
primitives.

So a typedef also helps with portability. There is nothing Rockbox specific
in my .lnk code, nor do I think it should be made non-portably Rockbox-only
if there's no need to do so.

Also, the typedef and the structs are only used within the called code, so
they should be transparent to any client programmer calling the supplied
code.


One could extend your argument by saying that structs of homogeneous types
are unnecessary, as
struct foo { long a ; long b; long c ; } ; could be replaced by long foo[
3 ] ;
but the advantage of named members is that it makes code and thus programmer
intent clearer.

Compare:
struct geo_point { long latitude_seconds ; long longitude_seconds } ;
int is_near_equator( geo_point g ) {
        return abs( g.latitude ) < 15 * 60 * 60 ;
}

to
// no struct or typedef
int is_near_equator( long gp[ 2 ] ) {
        return abs( gp[ 0 ] ) < 15 * 60 * 60
}

Of course, Rockbox isn't my project, so I'll accede to the opinion of the
Rockbox developers, but I do think there are good reasons to use typedef,
and that this is clearly one.


-----Original Message-----
From: owner-rockbox_at_cool.haxx.se [mailto:owner-rockbox_at_cool.haxx.se]On
Behalf Of Daniel Stenberg
Sent: Saturday, February 01, 2003 7:34 AM
To: Rockbox
Cc: jessehager_at_iname.com
Subject: Re: Windows sym-link code (standalone) for you to test


On Fri, 31 Jan 2003 rockbox_at_diffenbach.org wrote:

> I'm also interested in having my code critiqued for compliance with
> "best-practice" Rockbox code;

I'll focus my criticism on this topic. I'm a Linux user and I don't have any
.lnk files lying around to test. I do appreciate your efforts, please take
my
nits as friendly remarks on how to make your code better fit in with all the
existing code.

General formatting rules:

* We use plain old /* C comments */ and not // C++ comments
* Don't use tabs in source code.
* Indentation level is 4 spaces
* Please don't write source code that is wider than 79 columns or so

> typedef long dword ;
> typedef dword qword[ 2 ] ;

I'm not sure if you intend to keep these things, but in Rockbox we try to
avoid unnecessary typecasts of this kind that adds nothing but confusion.

Use 'long' instead of 'dword' and the qword can just be made a long array
with two entries.

> // end header
>
> #include <fcntl.h>
> #include<io.h>
> #include <stdio.h>
> #include <string.h>

We usually put all the include files at the top of the file.

> struct lnk_header_and_next_length header ;

...

> if( ( ret = read( file_handle, &header, sizeof( header ) ) ) != sizeof(
> header ) )
> return LNK_READ_ERROR ;

This kind of code I *think* will require that the struct is defined with
__attribute__((packed)) in gcc (see tools/bmp2rb.c) to work fine
crossplatform etc.

Again, I have not tested this or even attempted to follow the logic of the
code itself.

--
 Daniel Stenberg -- http://rockbox.haxx.se/ -- http://daniel.haxx.se/

Received on 2003-02-02