handful of bug fixes #76

ksajme · 2017-02-06T21:41:03Z

fixed select in roaring64map.hh not returning the upper 32 bits
fixed some incorrect printf format specifiers
made an impossible brance of an if possible
made compilation possible with DISABLE_X64 on MSVC 14.0
TODO: tests and benchmarks and ABM optmizations
removed .c file inline keyword

* fixed select in roaring64map.hh not returning the upper 32 bits * fixed some incorrect printf format specifiers * made an impossible brance of an if possible * made compilation possible with DISABLE_X64 on MSVC 14.0 * TODO: tests and benchmarks and ABM optmizations * removed .c file inline keyword

ksajme · 2017-02-06T21:42:58Z

cpp/roaring64map.hh

@@ -144,10 +144,10 @@ class Roaring64Map{
     * Check if value x is present
     */
    bool contains(uint32_t x) const {
-        return roarings.at(0).contains(x);
+        return roarings.count(0) == 0 ? false : roarings.at(0).contains(x);


rather big deal here, at() will throw an exception if we don't check count() first

ksajme · 2017-02-06T21:46:15Z

cpp/roaring64map.hh

        for (const auto& map_entry : roarings) {
            uint64_t sub_cardinality = (uint64_t)map_entry.second.cardinality();
            if (rank < sub_cardinality) {
-                return map_entry.second.select(rank, element);
+                *element = ((uint64_t)map_entry.first) << 32;


was ignoring the high-order bytes

ksajme · 2017-02-06T21:46:33Z

include/roaring/portability.h

@@ -27,11 +27,10 @@
 #include <malloc.h> // this should never be needed but there are some reports that it is needed.
 #endif

-#if __SIZEOF_LONG_LONG__ != 8
+#if defined(__SIZEOF_LONG_LONG__) && __SIZEOF_LONG_LONG__ != 8


not defined at all on Windows

@ksajme Interesting issue. Looks like we do not need to check that long long is 8 bytes in Visual Studio, even when compiling a 32-bit binary.

that's right, always 64

Anyhow, I think that this check is only needed on GCC-like compilers due to the intrinsics. GCC has intrinsics for int, long and long long. If long long is only 4 bytes, then we have no intrinsic for 8-byte values...

Still, for Visual Studio, I suspect that the same problem will occur... right? If you are compiling a 32-bit binary, you will be missing some 64-bit intrinsics, won't you?

Yes, in fact CMake will select 32 for a target by default. Caught me off-guard for this project. AFAIK there is no reliable way to force CMake to target 64 on Windows.

In 32-bit, compilation will fail while looking for built-ins. We could check for the existence of _WIN32 and the absence of _WIN64 if we wanted to print something friendlier.

@ksajme Can you do it as part of your PR? I'd imagine something like

#if defined (_WIN32) && !defined(_WIN64) #pragma message("You appear to be attempting a 32-bit build under Visual Studio. We recommend a 64-bit build instead.") #endif

General idea is to make a best-effort attempt at helping people.

Thoughts?

It's a good idea, especially since CMake makes it so easy to build as 32-bit by accident. I will move the defines around a bit so we don't catch MinGW or Clang on Windows.

FYI, it's not possible for "long long" to be <8 bytes. Both the C and C++ standards require that "long long" must be equal to or (in some far-flung future) greater than 8 bytes long.

So, while it might be useful as documentation for this check to be there, there's no architecture existing today that I'm aware of where it will be false (it would require a 128bit computer or something... and even then it's not entirely clear that the compiler implementers on those systems will choose to make "long long" be 128bits).

https://en.wikipedia.org/wiki/C_data_types
http://en.cppreference.com/w/cpp/language/types

@madscientist

As per your links, the "long long" specification comes from C99. Granted, that's what we expect, but it is not like this was built into the language decades ago. (For some reason, in C, C99 is still considered bleeding edge by some.)

So, while it might be useful as documentation for this check to be there, there's no architecture existing today that I'm aware of where it will be false

Ok. I am responsible for this check and it was indeed quite paranoid... but I am not sure it is harmful.

ksajme · 2017-02-06T21:47:35Z

include/roaring/portability.h

+/* Microsoft C/C++-compatible compiler */
+#include <intrin.h>
+
+/* wrappers for Visual Studio built-ins that look like gcc built-ins */


There are some faster versions of the built-ins used here which will function or be undefined depending on the processor features. Future work would be to identify the features and use the fancier built-ins

ksajme · 2017-02-06T21:48:01Z

src/containers/array.c

@@ -296,9 +296,9 @@ void array_container_printf_as_uint32_array(const array_container_t *v,
    if (v->cardinality == 0) {
        return;
    }
-    printf("%d", v->array[0] + base);
+    printf("%u", v->array[0] + base);


gcc found these incorrect format specifiers

ksajme · 2017-02-06T21:49:27Z

src/containers/bitset.c

@@ -534,7 +534,7 @@ uint16_t bitset_container_minimum(const bitset_container_t *container) {
 }

 /* Returns the largest value (assumes not empty) */
-inline uint16_t bitset_container_maximum(const bitset_container_t *container) {


This inline in a .c file chokes Visual Studio. I don't think it's doing anything in other C/C++ compilers since the implementation is not in the .h file so I removed it. Another fix would be to add the keyword and the implementation to the .h file.

Indeed, I don't think that this "inline" was correct.

ksajme · 2017-02-06T21:50:08Z

src/roaring_priority_queue.c

@@ -124,7 +124,7 @@ static roaring_bitmap_t *lazy_or_from_lazy_inputs(roaring_bitmap_t *x1,
            void *c;

            if ((container_type_2 == BITSET_CONTAINER_TYPE_CODE) &&
-                (container_type_2 != BITSET_CONTAINER_TYPE_CODE)) {
+                (container_type_1 != BITSET_CONTAINER_TYPE_CODE)) {


gcc found this impossible branch. This is a total guess of what it's supposed to be checking. Please correct me if I'm wrong!

@ksajme Yes, your fix is correct!

ksajme · 2017-02-06T21:51:01Z

tests/cpp_unit.cpp

@@ -154,6 +154,12 @@ void test_example_cpp(bool copy_on_write) {

    r2.printf();
    printf("\n");
+
+    // test select


Might as well publish the unit tests for select I was using to diagnose the problem in the 64-bit map

lemire · 2017-02-06T21:57:37Z

This looks like a high quality PR.

lemire · 2017-02-07T15:08:38Z

Merging.

ksajme commented Feb 6, 2017

View reviewed changes

Improved failure message for attempting to build 32-bit on Visual Studio

1b2ab15

lemire merged commit ec9ae21 into RoaringBitmap:master Feb 7, 2017

ksajme deleted the handfulOfBugFixes branch February 7, 2017 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handful of bug fixes #76

handful of bug fixes #76

ksajme commented Feb 6, 2017

ksajme Feb 6, 2017

ksajme Feb 6, 2017

ksajme Feb 6, 2017

lemire Feb 6, 2017

ksajme Feb 6, 2017

lemire Feb 6, 2017 •

edited

ksajme Feb 6, 2017

lemire Feb 6, 2017 •

edited

ksajme Feb 7, 2017

madscientist Feb 7, 2017

lemire Feb 7, 2017

ksajme Feb 6, 2017

ksajme Feb 6, 2017

ksajme Feb 6, 2017

lemire Feb 6, 2017

ksajme Feb 6, 2017

lemire Feb 6, 2017

ksajme Feb 6, 2017

lemire Feb 6, 2017

lemire commented Feb 6, 2017

lemire commented Feb 7, 2017

handful of bug fixes #76

handful of bug fixes #76

Conversation

ksajme commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemire Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemire Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemire commented Feb 6, 2017

lemire commented Feb 7, 2017

lemire Feb 6, 2017 •

edited

lemire Feb 6, 2017 •

edited