On 05/08/12 19:46, Daniel Wallace wrote:
> Signed-off-by: Daniel Wallace <daniel.wallace@gatech.edu>
> ---
> src/pacman/color.c | 728 ++++++++++++++++++++++++++++++++++++++++++++++++++ +++
> src/pacman/color.h | 70 ++++++
> src/pacman/util.c | 35 ++-
> src/pacman/util.h | 6 +
> 4 files changed, 830 insertions(+), 9 deletions(-)
> create mode 100644 src/pacman/color.c
> create mode 100644 src/pacman/color.h


BIG patch... so I am going to provide general comments.

Firstly, header for color.c is wrong, color.h is missing.


And here is the big one. There is a LOT of code "duplication". I say
"duplication" because some of the functions have diverged in the time
this patch has been around so they are no longer duplicates. That
should be an obvious maintenance burden we want to avoid.

An example:

color_yesno() vs. yesno():
- the only difference is the call to color_question() rather question()

then

color_question() vs. question():
- calls color_vfprintf() rather than vfprintf()


So, lets reduce this a lot.

I see that color_vfprintf falls back to just a standard vfprintf when
the "colors" variable is NULL. So we can just call this whenever this
is needed and pass NULL... in fact... see comment on the test below.



> +int color_vfprintf(FILE* stream, const colordata_t* colors, const
char* format, va_list args)

I'd call this vfprintf_color(). Well... I'd call it vfprintf_colour!
Anyway, I think teh vfprintf is the more important part so should be
read first.

> +{
> + int ret = 0;
> +
> + if(isatty(fileno(stream)) && colors) {

We could do:
if(config->color && colors && isatty(fileno(stream)))
then this could be a drop in replacement for vfprintf.

> + char* msg = NULL;
> + ret = vasprintf(&msg, format, args);
> + if(msg == NULL) {
> + return(ret);
> + }
> +
> + const colordata_t* colorpos = colors;

Huh?

> + color_t colorlast = COLOR_NONE;
> + int len = strlen(msg) + 1;
> + wchar_t* wcstr = calloc(len, sizeof(wchar_t));
> + len = mbstowcs(wcstr, msg, len);
> + free(msg);
> + const wchar_t *strpos = wcstr;
> +
> + while(*strpos) {
> + if(colorpos->color != COLOR_END &&
> + ((colorpos->separator == SEP_ANY) ||
> + (colorpos->separator == SEP_LINE && *strpos == L'
') ||
> + (colorpos->separator == SEP_COLON && (*strpos == L':' || *strpos
== L'：')))) {

What is that second character being tested?

> + _insert_color(stream, colorpos->color);
> + colorlast = colorpos->color;
> + colorpos++;
> + }
> + fprintf(stream, "%lc", (wint_t)*strpos);
> + strpos++;
> + }
> + free(wcstr);
> +
> + if(colorlast != COLOR_NONE) {
> + _insert_color(stream, COLOR_NONE);
> + }
> + } else {
> + ret = vfprintf(stream, format, args);
> + }
> + return(ret);
> +}


If similar things were done so that printf_color() etc all just fell
through to the non-colour version, the whole patchset could just be
printf -> printf_color changes.

Allan