A C style question and a review request (newbie alert)

Discussion in 'C Programming' started by chris.j.smith.uk@googlemail.com, Jan 30, 2009.

  1. Guest

    Hi,

    I've been writing C for a whopping 2 days and came up with this. Can
    some "experts" please have a poke at it with some constructive
    criticism so I know where I'm going wrong etc. It's a basic stack
    implementation with void pointers.

    http://78.86.255.5/stack.c.txt

    Compiles and works quite happily on GCC/Linux.

    Also pointers to things. What's the best notation:

    stack_t * stack;

    or

    stack_t* stack;

    ?

    I've got a 10 year background in C#/Java/VB/ASM etc but I've never
    used C for a moment (strangely). Decided to pick it up as it's more
    interesting and closer to the machine than what I do now.

    Cheers,

    Chris Smith
     
    , Jan 30, 2009
    #1
    1. Advertising

  2. "" <>
    writes:

    > I've been writing C for a whopping 2 days and came up with this. Can
    > some "experts" please have a poke at it with some constructive
    > criticism so I know where I'm going wrong etc. It's a basic stack
    > implementation with void pointers.
    >
    > http://78.86.255.5/stack.c.txt


    It looks good. BTW, it was not too large to post here and would have
    then have been easier to talk about.

    I'd have added a size function. In such a clean interface, why expect
    the "client" code to access sp->size? Secondly, I'd check the return
    of malloc. You also have a pointless cast in:

    | while((void*)stack_pop(stack) != NULL);

    stack_pop already returns void *. Some people like to make the null
    statement in that while more obvious, but that is a small style
    matter. I find the ; reasonably visible.

    You've used C99 possibly without knowing. In such simple code, there
    is no need to require anything more than the older standard. The
    feature you used is to mix declarations with statements.

    On the design: I'd probably reply on the point rather than the size to
    ensure that I can pop. It is, after all, the key data. The size is
    an extra in some sense. I'd be tempted to put unused nodes on a list
    rather than free'ing them, this avoiding a malloc when you next push.

    > Compiles and works quite happily on GCC/Linux.


    If you use: gcc -std=c99 -pedantic or gcc -std=c90 -pedantic you will
    not use features that are gcc specific or outside of your chosen
    version of C. Adding -Wall -Wextra can be good too.

    > Also pointers to things. What's the best notation:
    >
    > stack_t * stack;
    >
    > or
    >
    > stack_t* stack;
    >
    > ?


    you missed my preference: stack_t *stack; The * is syntactically
    associated with the name, not the type. Other people will have other
    opinions. BTW, the standard allows *_t names to be defined in the
    future so many people avoid them. t_* is an alternative.

    --
    Ben.
     
    Ben Bacarisse, Jan 30, 2009
    #2
    1. Advertising

  3. Guest

    On Jan 30, 4:43 pm, Ben Bacarisse <> wrote:
    > "" <>
    > writes:
    >
    > > I've been writing C for a whopping 2 days and came up with this.  Can
    > > some "experts" please have a poke at it with some constructive
    > > criticism so I know where I'm going wrong etc.  It's a basic stack
    > > implementation with void pointers.

    >
    > >http://78.86.255.5/stack.c.txt

    >
    > It looks good.  BTW, it was not too large to post here and would have
    > then have been easier to talk about.


    Thanks for the constructive remarks - much appreciated!

    Ok I shall include the source next time :)

    > I'd have added a size function.  In such a clean interface, why expect
    > the "client" code to access sp->size?  


    Ironically I did write a stack_size function as:

    int
    stack_size(stack_t *stack)
    {
    return stack->size;
    }

    I deleted it as I thought it was unnecessary. After thinking about it
    for a bit, I agree though and I suppose it helps encapsulation. I
    could move the proto into a header (which I will do when I've worked
    them out properly).

    > Secondly, I'd check the return
    > of malloc.  


    Good catch - I'll add that. Is there a policy for handling failed
    mallocs?

    > You also have a pointless cast in:
    >
    > | while((void*)stack_pop(stack) != NULL);


    That was crud left over from an earlier attempt. It started as a
    stack of int's but I discovered void pointers and added that in. I'll
    remove it.

    > stack_pop already returns void *.  Some people like to make the null
    > statement in that while more obvious, but that is a small style
    > matter.  I find the ; reasonably visible.


    I wasn't sure what was good there. Open for more suggestions and
    examples. I've seen for(;xxx;) used a bit but I don't like that -
    looked too crufty.

    > You've used C99 possibly without knowing.  In such simple code, there
    > is no need to require anything more than the older standard.  The
    > feature you used is to mix declarations with statements.


    Ok, reading ahead, I can trap that with GCC flags...

    > On the design: I'd probably reply on the point rather than the size to
    > ensure that I can pop.  It is, after all, the key data.  The size is
    > an extra in some sense.


    Size was only added to stop having to walk the list to determine the
    size. Premature optimisation and all that. I see your point - I'll
    check the tos == NULL to determine if a pop can be done. Actually, I
    can just return tos if it's null seeing as it's a void* ? This is
    making sense now. Thank you.

    > I'd be tempted to put unused nodes on a list
    > rather than free'ing them, this avoiding a malloc when you next push.


    Good plan. I'm going to write a dynamic string next (gap buffer) so
    I'll recycle the "list" components out of that back into this.

    > > Compiles and works quite happily on GCC/Linux.

    >
    > If you use: gcc -std=c99 -pedantic or gcc -std=c90 -pedantic you will
    > not use features that are gcc specific or outside of your chosen
    > version of C.  Adding -Wall -Wextra can be good too.


    I will try that. Better than my current makefile:

    gcc -g stack.c -o stack

    > > Also pointers to things.  What's the best notation:

    >
    > > stack_t * stack;

    >
    > > or

    >
    > > stack_t* stack;

    >
    > > ?

    >
    > you missed my preference:  stack_t *stack;  The * is syntactically
    > associated with the name, not the type.  Other people will have other
    > opinions.  BTW, the standard allows *_t names to be defined in the
    > future so many people avoid them.  t_* is an alternative.


    Excellent - just what I was after.

    Thanks for your help - much appreciated.

    Cheers,

    --
    Chris Smith
     
    , Jan 30, 2009
    #3
  4. wrote:

    > Hi,
    >
    > I've been writing C for a whopping 2 days and came up with this. Can
    > some "experts" please have a poke at it with some constructive
    > criticism so I know where I'm going wrong etc. It's a basic stack
    > implementation with void pointers.
    >
    > http://78.86.255.5/stack.c.txt
    >
    > Compiles and works quite happily on GCC/Linux.
    >
    > Also pointers to things. What's the best notation:
    >
    > stack_t * stack;
    >
    > or
    >
    > stack_t* stack;
    >
    > ?
    >
    > I've got a 10 year background in C#/Java/VB/ASM etc but I've never
    > used C for a moment (strangely). Decided to pick it up as it's more
    > interesting and closer to the machine than what I do now.


    There's one little problem:

    /* push element onto stack */
    void
    stack_push(stack_t* stack, void* value)
    {
    stack_element_t* entry;
    entry = malloc(sizeof(stack_element_t));
    entry->next = stack->tos;
    entry->payload = value;
    stack->tos = entry;
    stack->size++;
    }

    /* entry point */
    int
    main()
    {
    /* ... */
    stack_push(stack, "entry 1");
    stack_push(stack, "entry 2");
    stack_push(stack, "entry 3");
    /* ... */
    }

    The problem is, that only the addresses of the strings are copied on the
    stack. If those were dynamically allocated, and freed later on, you'd have
    invalid pointers on your stack. So either make sure, that the pointers you
    push on the stack don't go invalid, or copy the contents into newly
    allocated memory, which is to be freed after poping them off the stack. You
    can combine the latter with the usage of a garbage collector and don't
    explicitly free at all.

    Personally I'd go for copying the elements. If the elements contain pointers
    to dynamically allocated stuff itself, that must be dealt with, too.
    Actually I think garbage collectors make life a lot of easier and if used
    well they won't have any noticeable performance impact. At the point where
    the explicit cleanup code of a strict malloc+free

    I'm quite fond of the "Boehm GC" and use it throughout many projects.
    http://www.hpl.hp.com/personal/Hans_Boehm/gc/

    This is just a little detail, but a lot of newbies get this wrong. In your
    code things are just fine, but the following example is broken:

    void push_on_stack(stack_t * const stack)
    {
    char entry1[]="entry 1";
    char entry2[]="entry 2";
    char entry3[]="entry 3";

    stack_push(stack, entry1);
    stack_push(stack, entry2);
    stack_push(stack, entry3);
    }

    int main()
    {

    stack_t* stack;
    stack = stack_init();
    printf("size pre-push: %d\n", stack->size);

    push_on_stack(stack);

    printf("%s\n", (char*)stack_pop(stack));
    printf("%s\n", (char*)stack_pop(stack));
    printf("%s\n", (char*)stack_pop(stack));

    stack_destroy(stack);
    return 0;
    }

    In this example it's easy to see, what happens. The entry strings are local
    for push_on_stack. Once the scope of the function has been left, the
    entries pushed on the stack went invalid. The corrected version is:

    void push_on_stack(stack_t * const stack)
    {
    char entry1[]="entry 1";
    char entry2[]="entry 2";
    char entry3[]="entry 3";

    stack_push(stack, strdup(entry1));
    stack_push(stack, strdup(entry2));
    stack_push(stack, strdup(entry3));
    }

    int main()
    {
    chat *tmp;
    stack_t* stack;
    stack = stack_init();
    printf("size pre-push: %d\n", stack->size);

    push_on_stack(stack);

    tmp = (char*)stack_pop(stack);
    printf("%s\n", tmp);
    free(tmp);

    tmp = (char*)stack_pop(stack);
    printf("%s\n", tmp);
    free(tmp);

    tmp = (char*)stack_pop(stack);
    printf("%s\n", tmp);
    free(tmp);

    stack_destroy(stack);
    return 0;
    }

    Your initial programs works fine, since strings that are not used as an
    array initialization are placed in a global data section.

    Those two have completely different meaning:

    char this_array_is_initialized[] = "to this string";
    char *this_pointer_will_point_to_the_location_of = "this string";

    If used inside a function the first one will go invalid after terminating
    the function.

    Wolfgang
     
    Wolfgang Draxinger, Jan 30, 2009
    #4
  5. "" <>
    writes:

    > On Jan 30, 4:43 pm, Ben Bacarisse <> wrote:

    <snip>
    >> I'd have added a size function.  In such a clean interface, why expect
    >> the "client" code to access sp->size?  

    >
    > Ironically I did write a stack_size function

    <snip>
    > I deleted it as I thought it was unnecessary. After thinking about it
    > for a bit, I agree though and I suppose it helps encapsulation. I
    > could move the proto into a header (which I will do when I've worked
    > them out properly).


    If you do that you can then go one step further. By having only

    typedef struct stack_t stack_t;

    in the public header you can avoid revealing anything about the
    implementation. The full definition only need to go into the .c file
    than contains the code for the stack functions.

    >> Secondly, I'd check the return
    >> of malloc.  

    >
    > Good catch - I'll add that. Is there a policy for handling failed
    > mallocs?


    Not really. If this is general library code, returning some kind of
    failure would be normal. Calling exit is a possibility if this code
    is not going to be used anywhere where that would be a problem.

    <snip>
    >> stack_pop already returns void *.  Some people like to make the null
    >> statement in that while more obvious, but that is a small style
    >> matter.  I find the ; reasonably visible.

    >
    > I wasn't sure what was good there. Open for more suggestions and
    > examples. I've seen for(;xxx;) used a bit but I don't like that -
    > looked too crufty.


    I've seen:

    while (functioncall()) continue;

    and

    while (functioncall()) /* do nothing */;

    both with alternate layout. I don't think you need either.

    <snip>
    >> On the design: I'd probably reply on the point rather than the size to
    >> ensure that I can pop.  It is, after all, the key data.  The size is
    >> an extra in some sense.

    >
    > Size was only added to stop having to walk the list to determine the
    > size. Premature optimisation and all that. I see your point - I'll
    > check the tos == NULL to determine if a pop can be done.


    I am glad you penetrated the typos to get to the point I was making!

    --
    Ben.
     
    Ben Bacarisse, Jan 30, 2009
    #5
  6. CBFalconer Guest

    "" wrote:
    >

    .... snip ...
    >
    > Also pointers to things. What's the best notation:
    >
    > stack_t * stack;
    > or
    > stack_t* stack;


    You've answered your own question, if you look. We don't declare a
    pointer type. We declare a thing to be a pointer to a type. Thus
    you want:

    stack_p *stackitem;

    --
    [mail]: Chuck F (cbfalconer at maineline dot net)
    [page]: <http://cbfalconer.home.att.net>
    Try the download section.
     
    CBFalconer, Jan 31, 2009
    #6
  7. Richard Guest

    CBFalconer <> writes:

    > "" wrote:
    >>

    > ... snip ...
    >>
    >> Also pointers to things. What's the best notation:
    >>
    >> stack_t * stack;
    >> or
    >> stack_t* stack;

    >
    > You've answered your own question, if you look. We don't declare a
    > pointer type. We declare a thing to be a pointer to a type. Thus
    > you want:
    >
    > stack_p *stackitem;


    Disgusting. That looks like a pointer to a stack pointer.
     
    Richard, Jan 31, 2009
    #7
  8. Guest

    On Jan 31, 10:40 pm, Richard <> wrote:
    > CBFalconer <> writes:
    > > "" wrote:

    >
    > > ... snip ...

    >
    > >> Also pointers to things.  What's the best notation:

    >
    > >> stack_t * stack;
    > >>    or
    > >> stack_t* stack;

    >
    > > You've answered your own question, if you look.  We don't declare a
    > > pointer type.  We declare a thing to be a pointer to a type.  Thus
    > > you want:

    >
    > >   stack_p  *stackitem;

    >
    > Disgusting. That looks like a pointer to a stack pointer.


    I didn't like that either. I've settled on:

    I've decided to do the following:

    struct _stack { /* ... */ }; /* private impl */
    typedef struct _stack Stack; /* public interface */

    So code looks like:

    Stack *stack = stack_init();
    stack_destroy(stack);

    Still trying to decide if I like underscores...

    All changes and suggestions evaluated and applied. Thanks for all the
    help :)

    Cheers,

    Chris.
     
    , Jan 31, 2009
    #8
  9. "" <> writes:
    > On Jan 31, 10:40 pm, Richard <> wrote:
    >> CBFalconer <> writes:
    >> > "" wrote:

    >>
    >> > ... snip ...

    >>
    >> >> Also pointers to things.  What's the best notation:

    >>
    >> >> stack_t * stack;
    >> >>    or
    >> >> stack_t* stack;

    >>
    >> > You've answered your own question, if you look.  We don't declare a
    >> > pointer type.  We declare a thing to be a pointer to a type.  Thus
    >> > you want:

    >>
    >> >   stack_p  *stackitem;

    >>
    >> Disgusting. That looks like a pointer to a stack pointer.


    I think he meant "stack_t *stackitem;".

    > I didn't like that either. I've settled on:
    >
    > I've decided to do the following:
    >
    > struct _stack { /* ... */ }; /* private impl */
    > typedef struct _stack Stack; /* public interface */
    >
    > So code looks like:
    >
    > Stack *stack = stack_init();
    > stack_destroy(stack);
    >
    > Still trying to decide if I like underscores...


    Identifiers with leading underscores are reserved, and should be
    avoided in user code. It's a bit more complex than that; identifiers
    starting with an underscore and an uppercase letter, or with two
    underscores, are reserved in more contexts than other identifiers
    starting with underscores. But I find it's much easier just to avoid
    leading underscores altogether.

    Struct tags, like "_stack" in your example, are in their own
    namespace, so you don't have to worry about collisions with other
    identifers. So, if you like, you can just write:

    struct stack { /* ... */ };
    typedef struct stack stack;

    If the client code is going to make use of the fact that the type is a
    struct (in particular, if it's expected to refer to members of the
    struct), then my recommendation is not to use a typedef at all; just
    define the type as "struct stack" and refer to it that way.

    On the other hand, if it's supposed to be an opaque type, with client
    code using it only via pointers (like FILE in <stdio.h>), then you can
    do this:

    /* in stack.h */
    typedef struct stack Stack;
    /* "struct stack" is an incomplete type */
    Stack *New_Stack(void);

    /* in stack.c, not visible to client code */
    #include "stack.h"

    struct stack {
    /* ... */
    };

    Stack *New_Stack(void) {
    /* ... */
    }

    /* in some_program.c */
    #include "stack.h"

    Stack *my_stack = New_Stack();
    /* Attempting to declare an object of type Stack is illegal */

    Note that <stdio.h> doesn't use quite this approach for FILE, which is
    required to be an object type, not an incomplete type.

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    Nokia
    "We must do something. This is something. Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"
     
    Keith Thompson, Jan 31, 2009
    #9
  10. Richard Guest

    Keith Thompson <> writes:

    > "" <> writes:
    >> On Jan 31, 10:40 pm, Richard <> wrote:
    >>> CBFalconer <> writes:
    >>> > "" wrote:
    >>>
    >>> > ... snip ...
    >>>
    >>> >> Also pointers to things.  What's the best notation:
    >>>
    >>> >> stack_t * stack;
    >>> >>    or
    >>> >> stack_t* stack;
    >>>
    >>> > You've answered your own question, if you look.  We don't declare a
    >>> > pointer type.  We declare a thing to be a pointer to a type.  Thus
    >>> > you want:
    >>>
    >>> >   stack_p  *stackitem;
    >>>
    >>> Disgusting. That looks like a pointer to a stack pointer.

    >
    > I think he meant "stack_t *stackitem;".


    You think he meant? You now think FOR Chuck do you? Amazing.
     
    Richard, Feb 1, 2009
    #10
  11. wrote:

    > Also pointers to things. What's the best notation:
    >
    > stack_t * stack;
    >
    > or
    >
    > stack_t* stack;


    The best notation is:

    stack_t *stack;

    Why? Because of that:

    stack_t* stack_a, stack_b;

    means the very same as

    stack_t *stack_a; /* This is a pointer to stack_t */
    stack_t stack_b; /* That's just a stack_t */

    The '*' type qualifier is right associative, i.e. it only affects the next
    token right to it.

    While we're at it: 'const' is left associative. So

    int const foo = ...;

    and

    const int foo = ...;

    are the same. Personally I prefer the first variant. In a statement like the
    following

    int * const p;

    it means, that the value of the pointer can't be changed, i.e. it's address,
    but the contents at that address are mutable. Thus when defining string
    constants I prefer to do it like

    char const * const string_constant = "baz";

    which means that both the pointer, and the contents of where it points to
    are immutable.

    Wolfgang
     
    Wolfgang Draxinger, Feb 1, 2009
    #11
  12. Ian Collins Guest

    Wolfgang Draxinger wrote:
    > wrote:
    >
    >> Also pointers to things. What's the best notation:
    >>
    >> stack_t * stack;
    >>
    >> or
    >>
    >> stack_t* stack;

    >
    > The best notation is:


    There isn't a "best notation", it's purely subjective.

    > stack_t *stack;
    >
    > Why? Because of that:
    >
    > stack_t* stack_a, stack_b;
    >
    > means the very same as
    >
    > stack_t *stack_a; /* This is a pointer to stack_t */
    > stack_t stack_b; /* That's just a stack_t */


    Then write it this way.

    --
    Ian Collins
     
    Ian Collins, Feb 1, 2009
    #12
  13. Ian Collins wrote:

    > Wolfgang Draxinger wrote:
    >> wrote:
    >>
    >>> Also pointers to things. What's the best notation:
    >>>
    >>> stack_t * stack;
    >>>
    >>> or
    >>>
    >>> stack_t* stack;

    >>
    >> The best notation is:

    >
    > There isn't a "best notation", it's purely subjective.


    Of course in terms of the syntax there's no best way to do it.
    But there are style that are better readable than others. And
    the best test is, how well people new to the subject can read
    it.

    Hmm, let's to an experiment (I'm gonna do this with the
    introducory course in C I'm going to be tutoring):

    | Which of the following statements
    | is equivalent to "int* a, b, *c;"
    |
    | (a) int * a; int * b; int ** c;
    | (b) int * a; int b; int * c;
    | (c) int * a; int * b; int c;

    I bet, that 3 out of 5 will go for (a), 1 will go for (c) and
    only the guy who closely paid attention will choose the right
    one (b).

    > Then write it this way.


    I was never a friend of multiple declaration/definition, so I
    don't, but others do.

    Wolfgang
     
    Wolfgang Draxinger, Feb 1, 2009
    #13
  14. Phil Carmody Guest

    Wolfgang Draxinger <> writes:
    > Hmm, let's to an experiment (I'm gonna do this with the
    > introducory course in C I'm going to be tutoring):
    >
    > | Which of the following statements
    > | is equivalent to "int* a, b, *c;"


    That's roughly equivalent to
    """
    Dear Sir,
    Please accept my resignation.
    """

    > | (a) int * a; int * b; int ** c;
    > | (b) int * a; int b; int * c;
    > | (c) int * a; int * b; int c;
    >
    > I bet, that 3 out of 5 will go for (a), 1 will go for (c) and
    > only the guy who closely paid attention will choose the right
    > one (b).
    >
    >> Then write it this way.

    >
    > I was never a friend of multiple declaration/definition, so I
    > don't, but others do.


    If you're dealing with coordinates, then I see no benefit whatsoever
    from:
    int x;
    int y;
    int z;
    except to boost your productivity metrics.

    Not mixing pointers and non-pointers I can support, though, even
    though I quite happily do mix them in my own code, as I've always
    ensured there's enough redundancy in my variable names that there's
    not any confusion about what type a variable is.

    Phil
    --
    I tried the Vista speech recognition by running the tutorial. I was
    amazed, it was awesome, recognised every word I said. Then I said the
    wrong word ... and it typed the right one. It was actually just
    detecting a sound and printing the expected word! -- pbhj on /.
     
    Phil Carmody, Feb 2, 2009
    #14
  15. Ian Collins Guest

    Phil Carmody wrote:
    >
    > Not mixing pointers and non-pointers I can support, though, even
    > though I quite happily do mix them in my own code, as I've always
    > ensured there's enough redundancy in my variable names that there's
    > not any confusion about what type a variable is.


    Don't forget I've seen your code!

    --
    Ian Collins
     
    Ian Collins, Feb 2, 2009
    #15
  16. Phil Carmody wrote:

    > If you're dealing with coordinates, then I see no benefit
    > whatsoever from:
    > int x;
    > int y;
    > int z;
    > except to boost your productivity metrics.


    I'm quite oftenly programming with coordinates. But I usually
    don't put the components into separate variables as this can get
    inconvenient with certain libraries.

    Wolfgang
     
    Wolfgang Draxinger, Feb 2, 2009
    #16
  17. Phil Carmody Guest

    Ian Collins <> writes:
    > Phil Carmody wrote:
    >>
    >> Not mixing pointers and non-pointers I can support, though, even
    >> though I quite happily do mix them in my own code, as I've always
    >> ensured there's enough redundancy in my variable names that there's
    >> not any confusion about what type a variable is.

    >
    > Don't forget I've seen your code!


    Yeah, apologies. At least I have the excuse that that was Perl!

    Phil
    --
    I tried the Vista speech recognition by running the tutorial. I was
    amazed, it was awesome, recognised every word I said. Then I said the
    wrong word ... and it typed the right one. It was actually just
    detecting a sound and printing the expected word! -- pbhj on /.
     
    Phil Carmody, Feb 2, 2009
    #17
  18. Zach Guest

    On Jan 30, 11:26 am, ""
    <> wrote:
    > Hi,
    >
    > I've been writing C for a whopping 2 days and came up with this. Can
    > some "experts" please have a poke at it with some constructive
    > criticism so I know where I'm going wrong etc. It's a basic stack
    > implementation with void pointers.
    >
    > http://78.86.255.5/stack.c.txt
    >
    > Compiles and works quite happily on GCC/Linux.
    >
    > Also pointers to things. What's the best notation:
    >
    > stack_t * stack;
    >
    > or
    >
    > stack_t* stack;
    >
    > ?
    >
    > I've got a 10 year background in C#/Java/VB/ASM etc but I've never
    > used C for a moment (strangely). Decided to pick it up as it's more
    > interesting and closer to the machine than what I do now.
    >
    > Cheers,
    >
    > Chris Smith



    In the future please paste your code on a site like http://pastebin.com
    rather that your personal webserver attached to a dynamic IP on a
    residential connection. I can not access your code:

    The connection has timed out
    The server at 78.86.255.5 is taking too long to respond.

    Can you please paste it in the newsgroup.

    Zach
     
    Zach, Feb 4, 2009
    #18
  19. Guest

    On Feb 4, 9:30 pm, Richard Heathfield <> wrote:
    > Zach said:
    > > In the future please paste your code on a site like
    > >http://pastebin.comrather that your personal webserver attached
    > > to a dynamic IP on a residential connection.

    >
    > Better still, post it in your article, so that anyone reading the
    > article can also read the code.


    Original code as requested - this has been changed quite a bit since.
    I'll post an updated one soon.

    #include <stdio.h>
    #include <stdlib.h>

    /* stack element */
    typedef struct stack_element_t {
    void* payload;
    struct stack_element_t * next;
    } stack_element_t;

    /* stack */
    typedef struct stack_t {
    stack_element_t * tos;
    size_t size;
    } stack_t;

    /* initialise stack object */
    stack_t *
    stack_init()
    {
    stack_t* stack;
    stack = malloc(sizeof(stack_t));
    stack->tos = NULL;
    stack->size = 0;
    return stack;
    }


    /* pop value from stack */
    void*
    stack_pop(stack_t* stack)
    {
    stack_element_t* element;

    if (stack->size == 0)
    return NULL;

    element = stack->tos;
    stack->tos = element->next;
    void* result = element->payload;
    free(element);
    stack->size--;
    return result;
    }


    /* destroy stack */
    void
    stack_destroy(stack_t* stack)
    {
    while((void*)stack_pop(stack) != NULL);
    free(stack);
    }


    /* push element onto stack */
    void
    stack_push(stack_t* stack, void* value)
    {
    stack_element_t* entry;
    entry = malloc(sizeof(stack_element_t));
    entry->next = stack->tos;
    entry->payload = value;
    stack->tos = entry;
    stack->size++;
    }

    /* entry point */
    int
    main()
    {
    stack_t* stack;
    stack = stack_init();
    printf("size pre-push: %d\n", stack->size);
    stack_push(stack, "entry 1");
    stack_push(stack, "entry 2");
    stack_push(stack, "entry 3");
    printf("size post-push: %d\n", stack->size);
    printf("%s\n", (char*)stack_pop(stack));
    printf("%s\n", (char*)stack_pop(stack));
    printf("%s\n", (char*)stack_pop(stack));
    printf("size post-pop: %d\n", stack->size);
    stack_destroy(stack);
    return 0;
    }

    --
    Chris Smith
     
    , Feb 6, 2009
    #19
  20. Mark Wooding Guest

    Some of my points below apply to multiple parts of the program. I'm
    only going to mention them once each.

    "" <>
    writes:

    > #include <stdio.h>
    > #include <stdlib.h>
    >
    > /* stack element */


    This comment provides little value. I can guess that a type called
    `stack_element_t' is used to represent stack elements. (Particularly
    with syntax colouring editors, even `pointless' comments can serve to
    break up the monotony of a source file. Even so, I don't think I'd
    bother with this one.)

    > typedef struct stack_element_t {
    > void* payload;
    > struct stack_element_t * next;


    Your formatting of pointer declarators is inconsistent. I recommend:

    void *payload;
    struct stack_element_t *next;

    Reasons for preferring this form, with the `*' next to the name, have
    been discussed recently in other threads.

    > } stack_element_t;


    I'd (mildly) recommend against putting `_t' on the end of type names.
    Such names are reserved by POSIX (IEEE 1003.1--2001, 2.2.2) for
    implementations; while this isn't strictly a C issue, it does affect
    potential portability.

    > /* stack */
    > typedef struct stack_t {
    > stack_element_t * tos;
    > size_t size;
    > } stack_t;


    > /* initialise stack object */
    > stack_t *
    > stack_init()
    > {
    > stack_t* stack;
    > stack = malloc(sizeof(stack_t));


    I'd write

    stack = malloc(sizeof(*stack));

    here.

    > stack->tos = NULL;
    > stack->size = 0;


    You've not checked the return value from malloc. I don't have a problem
    with simply terminating the program on malloc failure, but others might
    -- but terminating the program and hoping that indirecting through a
    null pointer will crash it aren't the same. Of course, dealing with
    malloc failure more gracefully means that you'll have to decide on some
    way of reporting the error to the caller.

    > return stack;
    > }
    >
    >
    > /* pop value from stack */
    > void*
    > stack_pop(stack_t* stack)
    > {
    > stack_element_t* element;
    >
    > if (stack->size == 0)
    > return NULL;


    This is a fine check for stack-emptiness. However, I'd write it as

    if (!stack->tos)
    return (0);

    Why?

    * I have a strange habit of putting return values in parentheses.
    Once, a very long time ago, the parentheses were required, and this
    habit has stuck. I should have stopped doing this when I switched
    brace styles, but I didn't and I'm not going through another style
    change now.

    * I never use NULL because I consider it deceptive. Since NULL
    needn't expand to more than an unadorned `0', writing NULL as an
    argument to a variadic function which expects a pointer can cause
    undefined behaviour; but NULL looks like it ought to be safer than
    this. I find that writing `0' instead of `NULL' keeps me alert to
    these kinds of problems.

    * I've used `stack->tos' rather than `stack->size' to decide whether
    the stack is empty. If the stack is going to work at all properly,
    then stack->tos will be null when it's empty, and non-null
    otherwise. But stack->size is maintained as a secondary indicator
    and might -- conceivably -- get out of sync. Given this, it might
    be worth putting something like

    assert(!stack->tos == !stack->size);

    above. (This ties in with another recent thread about such
    conditions. Note the use of `!' to canonicalize the two sub-
    conditions to 0-or-1 truth values.)

    > element = stack->tos;
    > stack->tos = element->next;
    > void* result = element->payload;
    > free(element);
    > stack->size--;
    > return result;
    > }
    >
    >
    > /* destroy stack */
    > void
    > stack_destroy(stack_t* stack)
    > {
    > while((void*)stack_pop(stack) != NULL);


    Why cast the result of stack_pop to void * when it already returns one?

    I'd write

    while (stack_pop(stack));

    though some would like to see the semicolon on the next line:

    while (stack_pop(stack))
    ;

    which can make the empty loop body clearer. (I rely on my editor's
    automatic indentation to detect such things, and tend to trust the
    indentation of code I'm reading -- or beat it up with GNU Indent if it's
    obviously awful.)

    Notice that I've dropped the `!= NULL' part of the condition. I
    consider it redundant; but others may not.

    Moving on to a higher-level issue: presumably someone ought to have
    freed the individual stack-element payloads by now. You've just leaked
    them all. I think I'd actually replace the above loop by

    assert(!stack->tos);

    Then I can leave freeing of the stack elements to my caller, who
    presumably has a better idea of how it should be done, and can detect
    when the caller fails to do this.

    Oh, better

    #include <assert.h>

    at the top of the program. I don't believe in setting NDEBUG.

    > free(stack);
    > }



    > /* push element onto stack */
    > void
    > stack_push(stack_t* stack, void* value)
    > {
    > stack_element_t* entry;
    > entry = malloc(sizeof(stack_element_t));


    Again, you've not checked the return value from malloc. I said I wasn't
    going to repeat myself, but this is sufficiently important.

    I'd write

    stack_element_t *entry = malloc(sizeof(*entry));

    and save a line. Well, in truth, I wouldn't:

    stack_element_t *entry;
    if ((entry = malloc(sizeof(*entry))) != 0)
    /* ... do something about it ... */;

    (The `!= 0' is an old historical wart which prevents some old compilers
    from complaining about the assignment. Usually I'd omit that as
    redundant.)

    > entry->next = stack->tos;
    > entry->payload = value;
    > stack->tos = entry;
    > stack->size++;
    > }
    >
    > /* entry point */
    > int
    > main()
    > {
    > stack_t* stack;
    > stack = stack_init();
    > printf("size pre-push: %d\n", stack->size);
    > stack_push(stack, "entry 1");
    > stack_push(stack, "entry 2");
    > stack_push(stack, "entry 3");
    > printf("size post-push: %d\n", stack->size);


    Some may criticize you for exposing the stack length as a structure
    member rather than through a function; I don't see the problem myself.

    > printf("%s\n", (char*)stack_pop(stack));


    This cast isn't actually necessary (6.2.5p25). It's probably a good
    habit anyway: if it had been a pointer to anything other than character
    type, the cast would most definitely be necessary in this context.

    > printf("%s\n", (char*)stack_pop(stack));
    > printf("%s\n", (char*)stack_pop(stack));
    > printf("size post-pop: %d\n", stack->size);
    > stack_destroy(stack);
    > return 0;
    > }


    I'd say not bad overall. Please check the return value from malloc.

    -- [mdw]
     
    Mark Wooding, Feb 7, 2009
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Peter Karlsson

    Code Review Request: Newbie C programmer

    Peter Karlsson, Oct 16, 2004, in forum: C Programming
    Replies:
    15
    Views:
    1,134
    Richard Bos
    Oct 20, 2004
  2. Mersh
    Replies:
    0
    Views:
    524
    Mersh
    Mar 13, 2007
  3. www
    Replies:
    51
    Views:
    1,517
  4. Ganesh
    Replies:
    0
    Views:
    585
    Ganesh
    Jun 29, 2007
  5. Ken Varn
    Replies:
    0
    Views:
    530
    Ken Varn
    Apr 26, 2004
Loading...

Share This Page