From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 25 Jun 2013 01:10:43 GMT
Subject: Re: [PATCH 08/16] ewah: compressed bitmap implementation
Message-ID: <7v7ghj571o.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1372116193-32762-9-git-send-email-tanoku@gmail.com>

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).

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

Thanks for working on this.