Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

Well run projects I have contributed to have linters which fail on bad code style. Ask the submitter to make the linter happy before you review the code.




> Well run projects I have contributed to have linters which fail on bad code style. Ask the submitter to make the linter happy before you review the code.

Linters can't catch most things that are not syntax-style; i.e. linters can't catch semantic style.

Here is code CC generated this morning for me:

    size_t bvks_tls_read (bvks_tls_t *tls, void *dst, size_t dst_len)
    {
       int ret = wolfSSL_read (tls->ssl, dst, (int)dst_len);
    
       if (ret > 0)
          return (size_t)ret;
    
       if (ret == 0)
          return 0;
    
       // ret < 0, error case
       int err = wolfSSL_get_error (tls->ssl, ret);
       switch (err) {
          case WOLFSSL_ERROR_WANT_READ:
          case WOLFSSL_ERROR_WANT_WRITE:
             errno = EAGAIN;
             break;
          case WOLFSSL_ERROR_SYSCALL:
             // errno already set by the underlying socket call
             break;
          default:
             errno = EPROTO;
             break;
       }
       return (size_t)-1;
    }
    
    size_t bvks_tls_write (bvks_tls_t *tls, const void *src, size_t src_len)
    {
       int ret = wolfSSL_write (tls->ssl, src, (int)src_len);
    
       if (ret > 0)
          return (size_t)ret;
    
       if (ret == 0)
          return 0;
    
       // ret < 0, error case
       int err = wolfSSL_get_error (tls->ssl, ret);
       switch (err) {
          case WOLFSSL_ERROR_WANT_READ:
          case WOLFSSL_ERROR_WANT_WRITE:
             errno = EAGAIN;
             break;
          case WOLFSSL_ERROR_SYSCALL:
             // errno already set by the underlying socket call
             break;
          default:
             errno = EPROTO;
             break;
       }
       return (size_t)-1;
    }
Here is my preferred style:

    typedef int (tls_op_func_t) (WOLFSSL *, void *, int);
    
    static size_t tls_op (WOLFSSL *ssl, tls_op_func_t *fptr,
                          void *buf, size_t len)
    {
       if (len > INT_MAX) {
          errno = EINVAL;
          return (size_t)-1;
       }
    
       int wbytes = fptr (ssl, buf, (int)len);
       if (wbytes >= 0)
          return (size_t)wbytes;
    
       errno = map_wolf_error (wolfSSL_get_error (ssl, wbytes));
       return (size_t)-1;
    }
    
    size_t bvks_tls_read (bvks_tls_t *tls, void *dst, size_t dst_len)
    {
       return tls_op (tls->ssl, wolfSSL_read, dst, dst_len);
    }
    
    size_t bvks_tls_write (bvks_tls_t *tls, const void *src, size_t src_len)
    {
       return tls_op (tls->ssl, (tls_op_func_t *)wolfSSL_write, (void *)src, src_len);
    }
One of those is both hard to maintain and has precision (and potential overflow) bugs.

The other isolates the potentially buggy behaviour, validates it, and ensures that future changes to fix size_t/int precision loss bugs only has to be done in a single spot.

No linter is catching that style. It's more "coding style" than "syntax style".




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: