View Poll Results: What do you think of my code?

Voters
1. You may not vote on this poll
  • Easy to read, good standard C

    1 100.00%
  • Your code sucks

    0 0%

Style comments please

This is a discussion on Style comments please within the C Programming forums, part of the General Programming Boards category; Hi, everyone. I'm coding an open source project. Based on comments from here and IRC, I've rewritten part of it ...

  1. #1
    Registered User
    Join Date
    Feb 2011
    Posts
    126

    Style comments please

    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
    Last edited by Richardcavell; 04-27-2011 at 12:48 AM. Reason: Make links easier to browse

  2. #2
    Banned
    Join Date
    Aug 2010
    Location
    Ontario Canada
    Posts
    9,547
    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.

  3. #3
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,462
    Code:
    #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:
    Code:
    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.

  4. #4
    Registered User
    Join Date
    Feb 2011
    Posts
    126

    Reply

    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

  5. #5
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,666
    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:
    Quote Originally Posted by man 3 realloc
    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

  6. #6
    Registered User
    Join Date
    Oct 2008
    Posts
    19
    Quote Originally Posted by anduril462 View Post
    Code:
    #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:
    Code:
    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.
    Last edited by glennik; 04-27-2011 at 03:20 AM.

  7. #7
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,666
    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.

  8. #8
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,632
    Quote Originally Posted by glennik
    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.

    Quote Originally Posted by glennik
    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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  9. #9
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,494
    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
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  10. #10
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by Salem View Post
    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:

    Code:
    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.
    Last edited by MK27; 04-27-2011 at 05:19 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  11. #11
    Registered User
    Join Date
    Feb 2011
    Posts
    126
    Quote Originally Posted by Salem View Post
    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

  12. #12
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,494
    > 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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  13. #13
    Banned
    Join Date
    Aug 2010
    Location
    Ontario Canada
    Posts
    9,547
    Quote Originally Posted by Salem View Post
    > 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.

  14. #14
    Banned
    Join Date
    Aug 2010
    Location
    Ontario Canada
    Posts
    9,547
    Quote Originally Posted by Richardcavell View Post
    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.
    Last edited by CommonTater; 04-27-2011 at 11:19 AM.

  15. #15
    Registered User
    Join Date
    May 2009
    Posts
    2,549
    Quote Originally Posted by CommonTater View Post
    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.

Page 1 of 3 123 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Need some comments
    By shaizy in forum C Programming
    Replies: 0
    Last Post: 05-26-2006, 01:59 AM
  2. comments please...
    By kiranck007 in forum C Programming
    Replies: 4
    Last Post: 02-01-2006, 05:47 AM
  3. reinterpret_cast, C-style cast or function-style cast
    By blight2c in forum C++ Programming
    Replies: 3
    Last Post: 05-14-2002, 10:07 PM
  4. c-style string vs. c++-style strings
    By Mbrio in forum C++ Programming
    Replies: 3
    Last Post: 02-10-2002, 11:26 AM
  5. how do I extract a style from a DWORD style
    By zMan in forum Windows Programming
    Replies: 1
    Last Post: 01-17-2002, 09:09 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21