Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace roaring{,64}_bitmap_of with roaring{,64}_bitmap_from macro #570

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

Dr-Emann
Copy link
Member

@Dr-Emann Dr-Emann commented Jan 24, 2024

Using a macro allows us to count the number of items passed, and avoid having to explicitly pass the count as the first parameter.

@Dr-Emann Dr-Emann requested review from lemire and SLieve January 24, 2024 15:16
@Dr-Emann
Copy link
Member Author

Ah.. looks like maybe this wont work at all with msvc:

a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax

@lemire
Copy link
Member

lemire commented Jan 24, 2024

@Dr-Emann Let us see...

@lemire
Copy link
Member

lemire commented Jan 24, 2024

@Dr-Emann The problem is that the construction does not work in C++:

https://godbolt.org/z/4YrcYMEWT

msvc is happy to compile it in C:

https://godbolt.org/z/dcazf1rGM

include/roaring/roaring.h Outdated Show resolved Hide resolved
@lemire
Copy link
Member

lemire commented Jan 24, 2024

I think that this can work, one just needs to use different code when in C++. I can issue a PR against your PR if you don't immediately understand the issue.

include/roaring/roaring64.h Outdated Show resolved Hide resolved
Using a macro allows us to count the number of items passed, and avoid having
to explicitly pass the count as the first parameter.
@lemire
Copy link
Member

lemire commented Jan 24, 2024

Fantastic work.

@lemire lemire merged commit 413230b into master Jan 24, 2024
43 checks passed
@Dr-Emann Dr-Emann deleted the replace_bitmap_of branch January 24, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants