Nguyễn Thái Ngọc Duy <[email protected]> writes:
> diff --git a/graph.c b/graph.c
> index dd17201..048f5cb 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -62,6 +62,49 @@ enum graph_state {
> static const char **column_colors;
> static unsigned short column_colors_max;
>
> +static void set_column_colors(void)
When I said "'by config' sounds funny", I meant "'from config' may
be more natural". Perhaps name this read_graph_colors_config(), as
that (i.e. reading "log.graphColors") is what it does.
> +{
> + static char **colors;
> + static int colors_max, colors_alloc;
> + char *string = NULL;
> + const char *end, *start;
> + int i;
> +
> + for (i = 0; i < colors_max; i++)
> + free(colors[i]);
> + if (colors)
> + free(colors[colors_max]);
> + colors_max = 0;
The correctness of the first loop relies on the fact that colors is
non-null when colors_max is not zero, and then the freeing of the
colors relies on something else. It is not wrong per-se, but it
will reduce the "Huh?" factor if you wrote it like so:
if (colors) {
/*
* Reinitialize, but keep the colors[] array.
* Note that the last entry is allocated for
* reset but colors_max does not count it, hence
* "i <= colors_max", not "i < colors_max".
*/
int i;
for (i = 0; i <= colors_max; i++)
free(colors[i]);
colors_max = 0;
}
