roaring64: Add add_range, remove_range, contains_range, and flip

[SLieve]

This implements the following new functions:

• roaring64_bitmap_add_range
• roaring64_bitmap_remove_range
• roaring64_bitmap_contains_range
• roaring64_bitmap_flip
• roaring64_bitmap_flip_inplace
• roaring64_bitmap_flip_closed
• roaring64_bitmap_flip_closed_inplace

[lemire]

We are getting failures under Visual Studio. I could have a look if you cannot figure it out.

[SLieve]

It's only failing on win32, I think I'm probably missing a size specifier somewhere. If you could take a look that sounds great.

[lemire]

The problem is with roaring64_bitmap_of. If you call roaring64_bitmap_of(6,1,2,3,4,5,6)... You are assuming that the values are all uint64_t values. They are not, of course, in general. The safe usage is roaring64_bitmap_of(6,UINT64_C(1),UINT64_C(2),UINT64_C(3),UINT64_C(4),UINT64_C(5),UINT64_C(6)).

(Practically, roaring64_bitmap_of(6,1ULL,2ULL,3ULL,4ULL,5ULL,6ULL) might do, though it is technically incorrect. I would consider it acceptable within CRoaring.)

[SLieve]

That seems to have done it, thanks!

[Dr-Emann]

Ooof, that seems like a pretty big footgun.

Thoughts on something like

#define roaring64_bitmap_of(n_args, ...) \
    roaring64_bitmap_of_ptr((n_args), (const uint64_t[]){__VA_ARGS__})

[Dr-Emann]

Oh, or better:

#define roaring64_bitmap_of(...) \
    roaring64_bitmap_of_ptr((sizeof((const uint64_t[]){__VA_ARGS__}) / sizeof(uint64_t)), (const uint64_t[]){__VA_ARGS__})

No need to pass a number of args at all. And while __VA_ARGS__ occurs twice in expansion, one of the times is in a sizeof expression, which is an unevaluated context, so it's even safe in the case where expressions passed have side effects (roaring64_bitmap_of(my_func(), ++i))

[SLieve]

Oh, or better:

#define roaring64_bitmap_of(...) \
    roaring64_bitmap_of_ptr((sizeof((const uint64_t[]){__VA_ARGS__}) / sizeof(uint64_t)), (const uint64_t[]){__VA_ARGS__})

No need to pass a number of args at all. And while __VA_ARGS__ occurs twice in expansion, one of the times is in a sizeof expression, which is an unevaluated context, so it's even safe in the case where expressions passed have side effects (roaring64_bitmap_of(my_func(), ++i))

This sounds great (and easier to use), though it's a departure from the 32-bit API which could result in accidental misuse. Given that the 32-bit version is as it is, it would be better to keep them uniform IMO. Your initial proposal sounds good to me, happy to do that in a separate PR.

[Dr-Emann]

I agree, we can't have the 64 bit and 32 bit one different, and even with a major version bump, we can't change the behavior in such a way that would still compile but do something very different.

I can't even come up with a good name for the automatic sizing one, if we wanted to include it too, roaring_bitmap_of is just such a good name. The best I could come up with is roaring_bitmap_from(1, 2, 3).

[lemire]

@Dr-Emann roaring_bitmap_from sounds good.

[Dr-Emann]

I can do the PR for adding roaring(64)_bitmap_from. I'm okay with this PR as is