Introduce a property to get the pointer to the underlying bitmap

[Filip62]

This is useful for FFI.

I want to access the underlying C structure in another native extension through CFFI.

If this could be integrated, I would be grateful.

[Ezibenroc]

Hello,
Why not, thanks for the PR ! Could you please :

• Do the equivalent change in the 64 bit version ?
• Add a small unit test. For instance, calling this repeatedly on a same bitmap give the same result, but calling it on another bitmap gives a different result. Optionnally, it could be a (small) test where the pointer is dereferenced and some field of the bitmap is used.

[Filip62]

Yes, of course.
Both have been added.

[Ezibenroc]

Thanks ! One more change please. I would like to make it clear from both the field name and the docstrings (for both 32 and 64 bits) that this is risky territory.
For instance, the docstrings could look something like this:

The address of the underlying `roaring_bitmap_t`, as a Python int.

Intended for FFI interop, e.g. passing the bitmap to another
native extension via CFFI or ctypes without a copy.

Caveats (the caller is responsible for all of these):
  - The pointer is owned by this Python object. It is only valid
    while the object is alive; the caller must not pass it to
    `roaring_bitmap_free`.
  - Mutating the bitmap through this pointer on a `FrozenBitMap`
    is undefined behaviour: it silently invalidates the cached
    hash and breaks set/dict semantics.

The name ptr could be changed to c_pointer or raw_pointer.
These are examples, feel free to propose something else !

Thinking again about the tests, test_ffi_field_deref might obviously break if CRoaring changes its internal struct. So, I'm not so sure anymore it's a good idea to have it (I know I'm the one that suggested it, sorry). What is your opinion on this ? The other one (test_ffi_get_ptr) is great.

[Filip62]

Sure. I changed the docstrings and the property name.

I got rid of the field_deref test, I agree that it is fragile, but you asked for it so I went for it at first anyway.

But thinking about it a bit more, we do of course link the CRoaring library into the extension, so we can just interface with that using ctypes. So I added a straightforward test that just checks a change in cardinality using an FFI call.

[Ezibenroc]

So I added a straightforward test that just checks a change in cardinality using an FFI call.

Yes, good idea !

It fails on windows. From what I can read, it's because symbols are nor exported by default on windows. We could technically fix it, but it feels overkill for a simple test.
So, instead I suggest just skipping this test, something like this:

    @pytest.mark.skipif(sys.platform == "win32", "Symbols not exported on windows")
    def test_ffi_cardinality(self) -> None:

Apart from that, we should be good to merge.

[Filip62]

I've added the skip condition.

[Ezibenroc]

Thanks a lot, merging !

[Filip62]

Thank you too.