Junio C Hamano <gitster@xxxxxxxxx> writes: > Vicent Marti <tanoku@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html