Skip to main content
added 19 characters in body
Source Link
chux
  • 36.5k
  • 2
  • 43
  • 97
  1. With heavy use of macros,, care should be applied to insure exactly one use of a macro argument as the macro arguments could be complex and have side effects which only should be evaluated once.

     //#define ary_attach(ary, nbuf, nlen, nalloc)       \
     //        do {                                      \
     //                ary_freebuf(&(ary)->s);           \
     //                 (ary)->s.buf = (ary)->buf = nbuf;\
     // ...
     #define ary_attach(ary, nbuf, nlen, nalloc)       \
             do {                                      \
               struct aryb *ary__ = ary;  // add       \
               ary_freebuf(&ary__->s);                 \
               ary__->s.buf = ary__->buf = nbuf;       \
      ...
    
  2. Avoid magic numbers like 32. Why 32?

    // static const size_t snprintf_bufsize = 32;
    
    // Maximum buffer size of  string version of `int` log10(bitwidth)
    // Other formula are more precise, but better than guessing buffer needs.
    static const size_t snprintf_bufsize = sizeof(int)*CHAR_BIT/3 + 3;
    
    int ary_cb_inttostr(char **ret, const void *elem) {
      if (!(*ret = ary_xrealloc(NULL, snprintf_bufsize, 1)))
        return -1;
      return snprintf(*ret, snprintf_bufsize, "%d", *(int *)elem);
    }
    
  3. As free(NULL) is OK, IMO, any free-like function should also handle NULL.

     void ary_freebuf(struct aryb *ary) {
       if (ary == NULL) return; // add
       if (ary->len && ary->dtor) {
    
  4. ary.c should include files like strcmp.h and not count on #include "ary.h" to have included them.

  5. Consider simpler code, as below and in other places.

     // size_t seplen = sep ? strlen(sep) : 0, i, len;
     size_t seplen = sep ? strlen(sep) : 0;
     size_t i, len;
    
  6. Without clear code understanding, I have a bit concern about strbuf.buf[strbuf.len - 1]. Should strbuf.len == 0, disastrous result would occur.

  7. Uncertain: Comments on # lines is not portable. I'll have to research this.

     #endif /* ARY_H */
    
  8. Be careful about ary_xrealloc(), which calls realloc(). Should ary->len == 0 , return NULL is not an out-of-memory

     buf = ary_xrealloc(ary->buf, ary->len, ary->sz);
     // if (!buf) return 0;
     if (buf == NULL && ary->len > 0 && ary->sz > 0) return 0;
    
  1. With heavy use of macros,, care should be applied to insure exactly one use of a macro argument as the macro arguments could be complex and have side effects which only should be evaluated once.

     //#define ary_attach(ary, nbuf, nlen, nalloc)       \
     //        do {                                      \
     //                ary_freebuf(&(ary)->s);           \
     //                 (ary)->s.buf = (ary)->buf = nbuf;\
     // ...
     #define ary_attach(ary, nbuf, nlen, nalloc)       \
             do {                                      \
               struct aryb *ary__ = ary;  // add       \
               ary_freebuf(&ary__->s);                 \
               ary__->s.buf = ary__->buf = nbuf;       \
      ...
    
  2. Avoid magic numbers like 32. Why 32?

    // static const size_t snprintf_bufsize = 32;
    
    // Maximum buffer size of  string version of `int` log10(bitwidth)
    // Other formula are more precise, but better than guessing buffer needs.
    static const size_t snprintf_bufsize = sizeof(int)*CHAR_BIT/3 + 3;
    
    int ary_cb_inttostr(char **ret, const void *elem) {
      if (!(*ret = ary_xrealloc(NULL, snprintf_bufsize, 1)))
        return -1;
      return snprintf(*ret, snprintf_bufsize, "%d", *(int *)elem);
    }
    
  3. As free(NULL) is OK, IMO, any free-like function should also handle NULL.

     void ary_freebuf(struct aryb *ary) {
       if (ary == NULL) return; // add
       if (ary->len && ary->dtor) {
    
  4. ary.c should include files like strcmp.h and not count on #include "ary.h" to have included them.

  5. Consider simpler code, as below and in other places.

     // size_t seplen = sep ? strlen(sep) : 0, i, len;
     size_t seplen = sep ? strlen(sep) : 0;
     size_t i, len;
    
  6. Without clear code understanding, I have a bit concern about strbuf.buf[strbuf.len - 1]. Should strbuf.len == 0, disastrous result would occur.

  7. Uncertain: Comments on # lines is not portable. I'll have to research this.

     #endif /* ARY_H */
    
  1. With heavy use of macros,, care should be applied to insure exactly one use of a macro argument as the macro arguments could be complex and have side effects which only should be evaluated once.

     //#define ary_attach(ary, nbuf, nlen, nalloc)       \
     //        do {                                      \
     //                ary_freebuf(&(ary)->s);           \
     //                 (ary)->s.buf = (ary)->buf = nbuf;\
     // ...
     #define ary_attach(ary, nbuf, nlen, nalloc)       \
             do {                                      \
               struct aryb *ary__ = ary;  // add       \
               ary_freebuf(&ary__->s);                 \
               ary__->s.buf = ary__->buf = nbuf;       \
      ...
    
  2. Avoid magic numbers like 32. Why 32?

    // static const size_t snprintf_bufsize = 32;
    
    // Maximum buffer size of  string version of `int` log10(bitwidth)
    // Other formula are more precise, but better than guessing buffer needs.
    static const size_t snprintf_bufsize = sizeof(int)*CHAR_BIT/3 + 3;
    
    int ary_cb_inttostr(char **ret, const void *elem) {
      if (!(*ret = ary_xrealloc(NULL, snprintf_bufsize, 1)))
        return -1;
      return snprintf(*ret, snprintf_bufsize, "%d", *(int *)elem);
    }
    
  3. As free(NULL) is OK, IMO, any free-like function should also handle NULL.

     void ary_freebuf(struct aryb *ary) {
       if (ary == NULL) return; // add
       if (ary->len && ary->dtor) {
    
  4. ary.c should include files like strcmp.h and not count on #include "ary.h" to have included them.

  5. Consider simpler code, as below and in other places.

     // size_t seplen = sep ? strlen(sep) : 0, i, len;
     size_t seplen = sep ? strlen(sep) : 0;
     size_t i, len;
    
  6. Without clear code understanding, I have a bit concern about strbuf.buf[strbuf.len - 1]. Should strbuf.len == 0, disastrous result would occur.

  7. Uncertain: Comments on # lines is not portable. I'll have to research this.

     #endif /* ARY_H */
    
  8. Be careful about ary_xrealloc(), which calls realloc(). Should ary->len == 0 , return NULL is not an out-of-memory

     buf = ary_xrealloc(ary->buf, ary->len, ary->sz);
     // if (!buf) return 0;
     if (buf == NULL && ary->len > 0 && ary->sz > 0) return 0;
    
Source Link
chux
  • 36.5k
  • 2
  • 43
  • 97

Some lesser observations,

  1. With heavy use of macros,, care should be applied to insure exactly one use of a macro argument as the macro arguments could be complex and have side effects which only should be evaluated once.

     //#define ary_attach(ary, nbuf, nlen, nalloc)       \
     //        do {                                      \
     //                ary_freebuf(&(ary)->s);           \
     //                 (ary)->s.buf = (ary)->buf = nbuf;\
     // ...
     #define ary_attach(ary, nbuf, nlen, nalloc)       \
             do {                                      \
               struct aryb *ary__ = ary;  // add       \
               ary_freebuf(&ary__->s);                 \
               ary__->s.buf = ary__->buf = nbuf;       \
      ...
    
  2. Avoid magic numbers like 32. Why 32?

    // static const size_t snprintf_bufsize = 32;
    
    // Maximum buffer size of  string version of `int` log10(bitwidth)
    // Other formula are more precise, but better than guessing buffer needs.
    static const size_t snprintf_bufsize = sizeof(int)*CHAR_BIT/3 + 3;
    
    int ary_cb_inttostr(char **ret, const void *elem) {
      if (!(*ret = ary_xrealloc(NULL, snprintf_bufsize, 1)))
        return -1;
      return snprintf(*ret, snprintf_bufsize, "%d", *(int *)elem);
    }
    
  3. As free(NULL) is OK, IMO, any free-like function should also handle NULL.

     void ary_freebuf(struct aryb *ary) {
       if (ary == NULL) return; // add
       if (ary->len && ary->dtor) {
    
  4. ary.c should include files like strcmp.h and not count on #include "ary.h" to have included them.

  5. Consider simpler code, as below and in other places.

     // size_t seplen = sep ? strlen(sep) : 0, i, len;
     size_t seplen = sep ? strlen(sep) : 0;
     size_t i, len;
    
  6. Without clear code understanding, I have a bit concern about strbuf.buf[strbuf.len - 1]. Should strbuf.len == 0, disastrous result would occur.

  7. Uncertain: Comments on # lines is not portable. I'll have to research this.

     #endif /* ARY_H */