[ccan] clang warnings from ccan/rbtree/rbtree.c

[Rusty Russell]

On Tue, 8 Mar 2011 16:29:54 +1100, Brad Hards <bradh at frogmouth.net> wrote:
> I've been running the clang static analysis tool over ccan. Some issues, some 
> false alarms, some "hmm, what now".
> 
> Here is an extract from ccan/rbtree/rbtree.c:
> 
> /* setting a NULL node to black is a nop */
> static inline void trbt_set_color(trbt_node_t *node, int color)
> {
> 	if ( (node==NULL) && (color==TRBT_BLACK) ) {
> 		return;
> 	}
> 	node->rb_color = color;
> }
> static inline void trbt_set_color_left(trbt_node_t *node, int color)
> {
> 	if ( ((node==NULL)||(node->left==NULL)) && (color==TRBT_BLACK) ) {
> 		return;
> 	}
> 	node->left->rb_color = color;
> }
> static inline void trbt_set_color_right(trbt_node_t *node, int color)
> {
> 	if ( ((node==NULL)||(node->right==NULL)) && (color==TRBT_BLACK) ) {
> 		return;
> 	}
> 	node->right->rb_color = color;
> }
> 
> It only complains about the last one:
> 216  static inline void trbt_set_color_right(trbt_node_t *node, int color)
> 217  {
> 218          if ( ((node==NULL)||(node->right==NULL)) && (color==TRBT_BLACK) ) 
> {
>   1.  Assuming pointer value is null
>   2. Taking false branch
> 219                  return;
> 220          }
> 221          node->right->rb_color = color;
>   3.  Dereference of null pointer
> 222  }
> 
> That is, if node or node->right are NULL, and color is not equal to 
> TRBT_BLACK, then it will segfault. 
> 
> Now, I couldn't find a case where we ever pass anything other than a color of 
> TRBT_BLACK, but I'm still not sure what to do here:
> 1. Simplify the function so it only ever sets to black?
> 2. Change it to || instead of &&?
> 3. Ignore the whole thing?
> 
> Thoughts? Suggestions?

Well, step 1 is to CC the author :)

I would say: simplify.  But that may be naive...

Ronnie?

Cheers,
Rusty.