[prev in list] [next in list] [prev in thread] [next in thread] 

List:       squid-dev
Subject:    Re: interesting tidbit in =?UTF-8?Q?store=5Frepl=5Fheap=2Ecc?=
From:       Amos Jeffries <squid3 () treenet ! co ! nz>
Date:       2014-01-09 22:17:55
Message-ID: f24b0c506cc71d3df8b091f5c0f2eb5e () treenet ! co ! nz
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]