Re: interesting tidbit in store_repl_heap.cc

On 2014-01-10 04:14, Kinkie wrote:
> It's been there forever, and a build with the most recent clang
> version is failing on it:
>
> store_repl_heap.cc:224
> try_again:
>
> if (!heap_nodes(h->theHeap) > 0)
>
> Does anyone have any idea about what this code is supposed to do? A
> cursory look at the code would replace it with
> if (heap_empty(h->theHeap))
>
> but before doing that I'd like to make sure I didn't misunderstand.
>

Ew. Tricky little piece of garbage.

1) That nasty little ! operator has highest precedence just to confuse
things.
   So this is ((!heap_nodes(X)) > 0)

2) ! operator working on an integer of any kind is equivalent to (FOO ==
0)
   So this is ((heap_nodes(X) == 0) > 0)

3) implicit casting of boolean to integer true is non-0, false ==0.
   So this is (heap_nodes(X) == 0)

4) And then there is the heap_*(X) defines:
    #define heap_nodes(heap) ((heap)->last)
    #define heap_empty(heap) (((heap)->last <= 0) ? 1 : 0)

5) "last" is an unsigned value.
  So actually the define should be:
    #define heap_empty(heap) (((heap)->last == 0) ? 1 : 0)

6) == operator outputs a boolean
  So actually the define should be:
    #define heap_empty(heap) ((heap)->last == 0)

7) ==0 is equivalent to !
  So actually the define should be:
    #define heap_empty(heap) !((heap)->last)

So putting all these together the redux is:

A) the current form:

  (!heap_nodes(X)) > 0
  (!heap_nodes(X)) > 0
  (heap_nodes(X) == 0) > 0
  (heap_nodes(X) == 0)
  ((X)->last == 0)
  !((X)->last)

B) the proposed empty form:

  heap_empty(X)
  ((X->last <= 0) ? 1 : 0)
  (((X)->last == 0) ? 1 : 0)
  ((X)->last == 0)
  !((X)->last)

So yes I concur, the proposed replacement is equivalent. But it will
result in the if ((...)) syntax which clang very much disagrees with.

You will need to also fix the heap_empty macro definition to
!((heap)->last) just to get it to compile portably.

Amos
Received on Thu Jan 09 2014 - 22:18:02 MST