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

Have C++ defaults behave like stl defaults #193

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

brian-a-esch
Copy link
Contributor

  • Change bool ra_init() to void ra_init(), initializes array to have
    zero capacity. Does not malloc and cannot fail. ABI breaking change

  • Have Roaring ctor use new ra_init()

  • Make Roaring move ctor and move assignment noexcept. This is important
    for performance when holding them in stl containers (avoids copying)

  • Delete dtor, copy ctor, move ctor, copy assignment operator and move
    assignment operator for RoaringSetBitForwardIterator and Roaring64Map
    since these objects don't manage any lifetimes manually. The compiler
    provides these by default to us

  • Add static_asserts that C++ classes are noexcept moveable in cpp
    unit test file

@lemire
Copy link
Member

lemire commented Nov 12, 2018

See CI, we have...

c:\projects\croaring\tests\cpp_unit.cpp(24): error C2338: Expected Roaring64 to be no except move constructable [C:\projects\croaring\build\tests\cpp_unit.vcxproj]
713

*/
bool ra_init(roaring_array_t *t);
void ra_init(roaring_array_t *t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be marked noexcept as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure what you are asking.. That is a C function and cannot be marked as noexcept

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I got lost. You're right. GCC does have a nothrow attribute you can add to C functions but it's not necessary here.

@brian-a-esch
Copy link
Contributor Author

It appears that the move constructors on MSVC are not marked as noexcept. Here is a stack overflow https://stackoverflow.com/questions/47604029/move-constructors-of-stl-containers-in-msvc-2017-are-not-marked-as-noexcept

So the question is, do we want to conditionally do this static assert if we are on gcc or clang, or just remove it all together: https://github.com/RoaringBitmap/CRoaring/pull/193/files/9d0f97ae04ce4bfbed307ed358bbc225f7f088ab#diff-597bc881be4cdb305734afd4ac73dc2dR24

@lemire
Copy link
Member

lemire commented Nov 12, 2018

@Brian-Esch

Why do we need the static assert? That is, what is the harm in removing it?

@brian-a-esch
Copy link
Contributor Author

@lemire
The static assert is not necessary, I just thought it was a good way to test that the classes are noexcept moveable. I am going to remove the static assertion for Roaring64Map because the standard does not specify the noexcept requirements of a std::map's move ctor. I am also going to remove them for RoaringSetBitForwardIterator and Roaring64MapSetBitForwardIterator because it does not appear that iterators need to satisfy MoveConstructible: https://en.cppreference.com/w/cpp/named_req/Iterator.

Unless there are objections I would like to leave the static assert for the Roaring class, as a way of future proofing the code, since we have defined noexcept for it's move constructor

- Change `bool ra_init()` to `void ra_init()`, initializes array to have
  zero capacity. Does not `malloc` and cannot fail. ABI breaking change

- Have `Roaring` ctor use new `ra_init()`

- Make `Roaring` move ctor and move assignment `noexcept`. This is important
  for performance when holding them in stl containers (avoids copying)

- Delete dtor, copy ctor, move ctor, copy assignment operator and move
  assignment operator for `RoaringSetBitForwardIterator` and `Roaring64Map`
  since these objects don't manage any lifetimes manually. The compiler
  provides these by _default_ to us

- Add `static_assert` that `Roaring` is are `noexcept` moveable in cpp
  unit test file
@lemire
Copy link
Member

lemire commented Nov 14, 2018

This now looks good to me. Congrats!

@lemire lemire merged commit 0d09100 into RoaringBitmap:master Nov 14, 2018
Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.

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