[pacman-dev] [PATCH] Consolidate and improve table implementations

[Andrew Gregory]

On 06/30/13 at 10:53pm, Simon Gomizelj wrote:
> >>  
> >>  int trans_init(alpm_transflag_t flags, int check_valid)
> >> @@ -424,6 +432,60 @@ static size_t string_length(const char *s)
> >>  	return len;
> >>  }
> >>  
> >> +static void add_table_cell(alpm_list_t **row, char *label, int mode)
> >> +{
> >> +	struct table_cell_t *cell = malloc(sizeof(struct table_cell_t));
> >> +
> >> +	cell->label = label;
> >> +	cell->mode = mode;
> >> +	cell->len = string_length(label);
> >> +
> >> +	*row = alpm_list_add(*row, cell);
> >> +}
> >> +
> >> +static void table_free_cell(void *ptr)
> >> +{
> >> +	struct table_cell_t *cell = ptr;
> >> +
> >> +	if(cell) {
> >> +		if((cell->mode & CELL_FREE) && cell->label) {
> >> +			free(cell->label);
> > 
> > This isn't an official pacman style guideline and we do it in other
> > places, but I personally view NULL checks before calling free functions
> > as useless noise.  *free() functions should generally be NULL-safe, so
> > manually checking adds unnecessary clutter and can be confusing when
> > done inconsistently.
> > 
> 
> Its not NULL safe though. If cell is NULL, then cell->mode / cell->label
> is a null dereference error.
> 

I was referring to the cell->label check, not the if(cell)...

> >> +		}
> >> +		free(cell);
> >> +	}
> >> +}
> >> +
> >> +static void table_free(alpm_list_t *headers, alpm_list_t *rows)
> >> +{
> >> +	alpm_list_t *i;
> >> +
> >> +	alpm_list_free_inner(headers, table_free_cell);
> >> +
> >> +	for(i = rows; i; i = alpm_list_next(i)) {
> >> +		if(i->data) {
> >> +			alpm_list_free_inner(i->data, table_free_cell);
> >> +			alpm_list_free(i->data);
> > 
> > NULL check.
> > 
> 
> I'm assuming you don't want the check then?
>