Re: Request for review

Discussion in 'C Programming' started by James Antill, Jul 24, 2003.

  1. James Antill

    James Antill Guest

    On Thu, 24 Jul 2003 10:41:25 -0700, Bertrand Mollinier Toublet wrote:

    > Hi,
    >
    > I wrote an implementation of a dynamic array (i.e. you trade the
    > language syntax support for an array unbounded on one end).
    > I posted the code (including API specification, in the header file)
    > online at http://www.bmt.dnsalias.org/employment, in the project
    > section, at the bottom of the page.
    > Please review and comment /ad lib/.


    Well, it looks like it works :). Although you should check for overflow
    of the length in create/resize

    Some style comments, you have...

    typedef struct da_dyn_array_s da_dyn_array_t;
    typedef struct da_iterator da_iterator_t;

    1. I'd capitalize all typedef's due to them being in the same namespace as
    struct members.

    2. *_t is in the POSIX namespace, so you might want to not usre that
    suffix.

    3. The _s suffix on the struct should be on both, or my preference would
    be not at all.

    The implementation of db_create is...

    da_dyn_array_t *new_da;

    new_da = malloc(sizeof *new_da);
    if (NULL != new_da)
    {
    new_da->base = malloc(nmemb * size);
    if (NULL == new_da->base)
    {
    free(new_da);
    new_da = NULL;
    }
    else
    {
    new_da->nmemb = nmemb;
    new_da->size = size;
    new_da->index = 0;
    }
    }
    return new_da;

    ....this looks much nicer as (esp. with the extra checks)...

    da_dyn_array_t *new_da = NULL;
    size_t bytes = 0;

    new_da = malloc(sizeof *new_da);
    if (NULL == new_da)
    goto malloc_da_fail;

    if (!nmemb) /* don't allow this, so we can check the args */
    nmemb = 1; /* also gets rid of corner case in resize */

    bytes = nmemb * size;
    if ((0 == bytes) || ((bytes / size) != nmemb))
    goto check_args_fail;

    new_da->base = malloc(nmemb * size);
    if (NULL == new_da->base)
    goto malloc_base_fail;

    new_da->nmemb = nmemb;
    new_da->size = size;
    new_da->index = 0;

    return new_da;

    malloc_base_fail:
    check_args_fail:
    free(new_da);

    malloc_da_fail:
    return (NULL);

    ....but as I said, they aren't required ... just comments.

    --
    James Antill --
    Need an efficent and powerful string library for C?
    http://www.and.org/vstr/
     
    James Antill, Jul 24, 2003
    #1
    1. Advertising

  2. James Antill wrote:
    > On Thu, 24 Jul 2003 10:41:25 -0700, Bertrand Mollinier Toublet wrote:
    >
    >
    >>Hi,
    >>
    >>I wrote an implementation of a dynamic array (i.e. you trade the
    >>language syntax support for an array unbounded on one end).
    >>I posted the code (including API specification, in the header file)
    >>online at http://www.bmt.dnsalias.org/employment, in the project
    >>section, at the bottom of the page.
    >>Please review and comment /ad lib/.

    >
    >
    > Well, it looks like it works :). Although you should check for overflow
    > of the length in create/resize
    >
    > bytes = nmemb * size;
    > if ((0 == bytes) || ((bytes / size) != nmemb))
    > goto check_args_fail;
    >

    James,

    thanks for the review. Checking for overflow definitely is a good idea.
    Although, I found the following in the bugs list of your library:

    .. size overflows, just don't do it (tm) ... (Ie. having a Vstr string of
    length (SIZE_MAX - 1) and adding 2 bytes to it will just break.

    What's up with that ;-) ?


    --
    Bertrand Mollinier Toublet
    "Uno no se muere cuando debe, sino cuando puede"
    -- Cor. Aureliano Buendia
     
    Bertrand Mollinier Toublet, Jul 25, 2003
    #2
    1. Advertising

  3. James Antill

    James Antill Guest

    On Fri, 25 Jul 2003 09:11:46 +0000, Dan Pop wrote:

    > In <> "James Antill" <> writes:
    >
    >>1. I'd capitalize all typedef's due to them being in the same namespace as
    >>struct members.

    >
    > Where did you get this idea from?


    Hmm, I thought gcc had warned me for having a memeber the same as a
    typedef I was getting from the zlib library at one point ... but all
    recent versions don't seem to. The only thing I can find in c99 is 6.7.7.6
    but gcc doesn't support unnamed memebers even now and the override in the
    global variable isn't illegal, although gcc does warn about that.

    Looks like that's not true.

    --
    James Antill --
    Need an efficent and powerful string library for C?
    http://www.and.org/vstr/
     
    James Antill, Jul 26, 2003
    #3
    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. Jim Lewis
    Replies:
    0
    Views:
    492
    Jim Lewis
    Feb 28, 2005
  2. Brian Birtle
    Replies:
    2
    Views:
    2,193
    John Saunders
    Oct 16, 2003
  3. Rick

    Request for peer review.

    Rick, Jan 11, 2004, in forum: ASP .Net
    Replies:
    0
    Views:
    324
  4. Rick
    Replies:
    1
    Views:
    465
    Cowboy \(Gregory A. Beamer\)
    Jan 15, 2004
  5. www
    Replies:
    51
    Views:
    1,519
Loading...

Share This Page