Re: [evolution-patches] Patch for the bug #66523

[Hans Petter Jansson <hpj ximian com>]

On Wed, 2004-09-22 at 11:47 +0800, shenghao wrote:

> Attach is the correct patch to fix the bug #66523 on 
> http://bugzilla.ximian.com.
> Summary : "No error dialog in contact list"
> 
> Would you like to spend a little time to review it, please?

Some comments:

- The #include statements you are adding should be further down on the
  list of includes - after config.h and system includes. Place them
  among the other gtk/libgnome includes.

- You are declaring variables after the preconditions in the function.
  This is C99, which we do not use due to portability issues. Please
  declare all variables at the start of the scope.

- Use diff -up to generate diffs. The -p adds function names to context,
  making the patch more readable.

- Leave a blank line between variable declarations and the first
  non-declaration statement (i.e. between "gint response;" and
  "compare = FALSE;").

- Leave a space between a cast and whatever follows:

  (char *) e_table_model_value_at (E_TABLE_MODEL (model), ...);

- Leave a space between the name of a function you're calling and the
  opening parenthesis for its parameters. You're doing this in some
  places, but not all.

- You can use for () instead of while () and integrate row++ into the
  loop logic:

  for (row = 0; row < row_count; row++)

- You don't need the 'compare' variable. Instead, if the
  strcmp (list, email) test succeeds, issue a break if the user wants
  to add the address anyway. That way, you don't needlessly iterate
  over the rest of the list, and the user won't get any more prompts,
  either.

-- 
Hans Petter