Re: [PATCH 08/16] ewah: compressed bitmap implementation

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Vicent Marti <tanoku@gmail.com> writes:
>
>> The library is re-licensed under the GPLv2 with the permission of Daniel
>> Lemire, the original author. The source code for the C version can
>> be found on GitHub:
>>
>> 	https://github.com/vmg/libewok
>>
>> The original Java implementation can also be found on GitHub:
>>
>> 	https://github.com/lemire/javaewah
>> ---
>
> Please make sure that all patches are properly signed off.
>
>>  Makefile           |    6 +
>>  ewah/bitmap.c      |  229 +++++++++++++++++
>>  ewah/ewah_bitmap.c |  703 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  ewah/ewah_io.c     |  199 +++++++++++++++
>>  ewah/ewah_rlw.c    |  124 +++++++++
>>  ewah/ewok.h        |  194 +++++++++++++++
>>  ewah/ewok_rlw.h    |  114 +++++++++
>
> This is lovely.  A few comments after an initial quick scan-through.
>
>  - The code and the headers are well commented, which is good.
>
>  - What's __builtin_popcountll() doing there in a presumably generic
>    codepath?
>
>  - Two variants of "bitmap" are given different and easy to
>    understand type names (vanilla one is "bitmap", the clever one is
>    "ewah_bitmap"), but at many places, a pointer to ewah_bitmap is
>    simply called "bitmap" or "bitmap_i" without "ewah" anywhere,
>    which waas confusing to read.  Especially, the "NAND" operation
>    for bitmap takes two bitmaps, while "OR" takes one bitmap and
>    ewah_bitmap.  That is fine as long as the combination is
>    convenient for callers, but I wished the ewah variables be called
>    with "ewah" somewhere in their names.
>
>  - I compile with "-Werror -Wdeclaration-after-statement"; some
>    places seem to trigger it.
>
>  - Some "extern" declarations in *.c sources were irritating;
>    shouldn't they be declared in *.h file and included?
>
>  - There are some instances of "if (condition) stmt;" on a single
>    line; looked irritating.   
>
>  - "bool" is not a C type we use (and not a particularly good type
>    in C++, either).

One more.

  - Use of unnecessary float (e.g. "oldval *= 1.5") were moderately
    annoying.


> That is it for now. I am looking forward to read through the users
> of the library ;-)
>
> Thanks for working on this.