Re: [dev] [sbase][PATCH v2] diff

From: Mattias Andrée <maandree_AT_kth.se>
Date: Sun, 31 Jan 2016 19:08:37 +0100

On Sun, 31 Jan 2016 12:39:34 +0100
Hiltjo Posthuma <hiltjo_AT_codemadness.org> wrote:

> Some notes from me:
>
> - I would avoid using variadic arrays, use a sane subset
> of C99.

Are you refering to this code?

  #define matrix (*matrix)
  #define map (*map)
  char map[an + 1][bn + 1] = emalloc(sizeof(char[an + 1][bn + 1]));
  size_t matrix[2][bn + 1] = ecalloc(1, sizeof(size_t[2][bn + 1]));

Perhaps the #define:s makes it confusing, but I though the code
beneath it should be simplified by them and that it was worth it.
I was suggested to use

  size_t (*matrix)[n][m] to simplify the matrix allocation.

Remove the #define:s and write (*) explicitly?

> - Use snprintf, instead of sprintf and the check.

Are you refering to the this code?

  sprintf(buf + (sizeof("0000-00-00 00:00:00.") - 1), "%09lu", attr->st_mtim.tv_nsec);

There are no checks, and the buffer is guaranteed
to be large enough, so I disagree about using snprintf.

> - Use strlcpy or snprintf instead of strcpy.
> - Avoid using the "inline" #defines.

I will look into these.

> - Disable colour output by default, maybe just add a flag
> for it to explicitly enable it.

Does it matter if stdout is a TTY? Anyway I rather remove
it than add another flag. But lets see what everyone thinks.

> - We should also allow using stdin for one of the file
> inputs (so can't "reread" file): diff file.c -

stdin (via -) should be supported.

>
> Kind regards,
> Hiltjo
>


Received on Sun Jan 31 2016 - 19:08:37 CET

This archive was generated by hypermail 2.3.0 : Sun Jan 31 2016 - 19:12:10 CET