Style comments please

[Richardcavell]

Hi, everyone. I'm coding an open source project. Based on comments from here and IRC, I've rewritten part of it with a new approach to style. I'm looking for feedback. Do you find this code easy to understand? Please look specifically at comtypes.c and comtypes.h (I haven't worked on the other source files yet).

http://autobotwiki.svn.sourceforge.n...32&view=markup
http://autobotwiki.svn.sourceforge.n...32&view=markup

[CommonTater]

Well... if I ignore obvius errors like only allocating 1 byte of memory for a rather large struct and silly errors like misplaced modifiers ... I suppose you're getting better at it.

At least you don't have 8232578 global variables floating around anymore.

[anduril462]

#ifndef AUTOBOT_COMTYPES_H

You gonna give a shout out to the Decepticons too?

Markedly improved over your first attempts. Can I convince you to avoid unnecessary casts, like these two:

free ((void *) get_text (buffer));
...
set_text (buffer, (char *) malloc (1));

The rest of it looks fine on the surface, but it's too late for me to do a serious review.

[Richardcavell]

1. The malloc(1) sets up a text buffer. The struct does not go into the buffer, the pointer to the buffer goes into the struct. The pointer is subsequently realloc()ed as necessary as the buffer is filled, *if* it is filled at all. (I ran tests and it's better this way).

2. What modifiers are misplaced? The casts to and from malloc()/free() are to stop a C++ compiler from complaining. Several people claim to want to use my code as C++, so I'm writing in a version of C that will compile unmodified as C++.

3. Yes, the name of my project is partly inspired by the Transformers.

Richard

[whiteflags]

1. The malloc(1) sets up a text buffer. The struct does not go into the buffer, the pointer to the buffer goes into the struct. The pointer is subsequently realloc()ed as necessary as the buffer is filled, *if* it is filled at all. (I ran tests and it's better this way).

I don't see the point. In case you didn't know:

If ptr is NULL, the call is equivalent to malloc(size); if size is equal to zero, the call is equivalent to free(ptr). Unless ptr is NULL, it must have been returned by an earlier call to malloc(), calloc() or realloc().

realloc(3): Allocate/free dynamic memory - Linux man page

[glennik]

#ifndef AUTOBOT_COMTYPES_H

You gonna give a shout out to the Decepticons too?

Markedly improved over your first attempts. Can I convince you to avoid unnecessary casts, like these two:

free ((void *) get_text (buffer));
...
set_text (buffer, (char *) malloc (1));

These casts are not unnecessary, but rather highly clarifying, indicating that that this is a willed and controlled cast. In a such low rigid language like C, where you basically are allowed to do almost everything, you are left with the programmers knowledge and workmanship for stable and maintainable code. Un-controlled or unreflected cast makes up a huge quantities of bugs in traditional C programs, and if you use the pedantic flag of the GCC or LINT, they will both kick your but for NOT having these strict casts.

In a strong typed language, like Java, cast like this are mandatory, otherwise it will not compile. So value the freedom of programming in C, but realize that the price is a huge number of pitfalls, and non-strict casts are one of the major ones.

[whiteflags]

These casts are not unnecessary, but rather highly clarifying, indicating that that this is a willed and controlled cast. ... and if you use the pedantic flag of the GCC or LINT, they will both kick your but for NOT having these strict casts.

lint maybe, but the compiler only complains about the lack of casting if you neglect to include the relevant header in this case, the stdlib.h because the compiler would assume malloc returns an int and free could theoretically take anything since it's assumed declaration is int free(); in those instances. That is the only time you need to cast using those functions to indicate not just to programmers but to the compiler that a conversion is wanting to take place.

Further this FAQ seems to imply that casting itself is bad in C, (if you don't know what you're doing) because it isn't as stringent about what is being cast, unlike other languages, which only permit certain casts to do certain conversions. If you can write C code without casting anything, the compiler will warn you about bad typing.

[laserlight]

These casts are not unnecessary, but rather highly clarifying, indicating that that this is a willed and controlled cast. In a such low rigid language like C, where you basically are allowed to do almost everything, you are left with the programmers knowledge and workmanship for stable and maintainable code. Un-controlled or unreflected cast makes up a huge quantities of bugs in traditional C programs

The problem is that in the absence of a function declaration (e.g., failure to include a header), the return type of a function that is called is assumed to be int. This mistake would be masked by the cast.

if you use the pedantic flag of the GCC or LINT, they will both kick your but for NOT having these strict casts.

I don't know about Lint, but gcc does not warn about a "missing" cast to char* with -Wall and -pedantic. Besides, pedantically speaking, conversion from a pointer to object type to a pointer to void is always safe.

[Salem]

Mmm, all I get is

An Exception Has Occurred

Unknown location: /comtypes.c
HTTP Response Status

404 Not Found

/Edit
Go on holiday, ask for room 404, then hassle the check-in staff that you can't find it

[MK27]

Mmm, all I get is
Go on holiday, ask for room 404, then hassle the check-in staff that you can't find it

Me too, oh well. @Richard: great idea to do this. I sure wish I had gotten some criticism when I wrote my first open source project. I have been tidying it up ever since. Check out the unique formatting style in this little gem:

char *linein (FILE *stream) {   /* uses malloc */ 
        int c=0;                
        char *line = NULL, buffer[1024], byte, kb=0, *tmp;
        memset(buffer,0,1024);

        if (Debug>2) { g_print("linein()..."); fflush(stdout); }
        
        while (fread(&byte,1,1,stream)==1) {
                buffer[c]=byte;
                c++;
                if (byte=='\n') break;
                if (c==1024) {
                        kb++;
                        if (kb>1) { if (!(tmp=realloc(line,1024*kb))) { fprintf(stderr, "!!!%s linein() REALLOC FAILED, OUT OF MEMORY\n", Me);
 return line; }
                                line=tmp; }
                        else { line=ec_malloc(strlen(buffer)+1); line[0]='\0'; }
                        strcat(line,buffer);
                        memset(buffer,0,1024);
                        c=0;
                }                               
        }               
        if (c==0) return line;
        if (kb) { if (!(tmp=realloc(line,kb*1024+strlen(buffer)+1))) {
                fprintf(stderr, "!!!%s linein() REALLOC FAILED, OUT OF MEMORY\n", Me); return line; }
                line=tmp;
        } else line=ec_malloc(strlen(buffer)+1);
        strcat(line,buffer);
        if (Debug>2) { g_print("%s",line); fflush(stdout); }
        return line;
}

...from version 0.7.1. This goes on for 3-4000 lines. I actually have to avert my eyes when I see the list of globals.

[Richardcavell]

Mmm, all I get is


/Edit
Go on holiday, ask for room 404, then hassle the check-in staff that you can't find it

Salem,

Are you compiling as C++? I don't see how you're getting exceptions otherwise. Although my code is intended to compile as C++, I didn't say I was there yet. It's on the TODO list. The bot works perfectly for me from both Ubuntu and OS X.

Richard

[Salem]

> Are you compiling as C++?
No, I'm using a browser to point to your URL.

Haven't got anywhere near seeing the code to compile it.

[CommonTater]

> Are you compiling as C++?
No, I'm using a browser to point to your URL.

Haven't got anywhere near seeing the code to compile it.

It was there yesterday... gone today.

[CommonTater]

1. The malloc(1) sets up a text buffer. The struct does not go into the buffer, the pointer to the buffer goes into the struct. The pointer is subsequently realloc()ed as necessary as the buffer is filled, *if* it is filled at all. (I ran tests and it's better this way).

Even though it could easily result in a memory leak?
You should set the pointer to NULL until you have something to put into it and again after you free() the memory.
The behavior of realloc() in this regard has already been explained to you.

[stahta01]

It was there yesterday... gone today.

Yeah, but it is in SVN

SourceForge.net Repository - [autobotwiki] Contents of /comtypes.c
SourceForge.net Repository - [autobotwiki] Contents of /comtypes.c

Tim S.