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

C++ implementation should be namespaced #197

Closed
jbayardo opened this issue Jan 8, 2019 · 4 comments
Closed

C++ implementation should be namespaced #197

jbayardo opened this issue Jan 8, 2019 · 4 comments

Comments

@jbayardo
Copy link

jbayardo commented Jan 8, 2019

If you use CRoaring as it comes in the library, it generates a name clash with the Point Cloud Library. This is because of the #include "roaring.h" line in the head of roaring.hh, causing it to bring many functions into the global namespace.

I have "fixed this" by creating

roaring.h

#pragma once

namespace roaring {
#include <roaring/roaring.hh>
}

And including that directly, but it seems like this should be handled library-level.

@lemire
Copy link
Member

lemire commented Jan 8, 2019

Yes, that’s a good point.

Do you want to issue a PR?

@AE1020
Copy link
Contributor

AE1020 commented Oct 27, 2020

I have already patches to get the entirety of roaring to build as C++, which corrects issues like spanning header files with extern "C" { }.

Namespaces similarly should not span headers and only roaring-specific content, so they can go at those locations. So:

 #ifdef __cplusplus
 extern "C" { namespace roaring {
 #endif

I first tried everything under one namespace, which works but it would lead people to saying using namespace roaring; when they want much less.

Then split this up into ::roaring for the C++ exports, ::roaring::api for the C public API vs. ::roaring::internal for service routines.

To the extent anyone knows what namespace best practices are, it tries to follow them by not doing using statements in headers unless it has to for C compatibility.

@lemire
Copy link
Member

lemire commented Oct 27, 2020

@AE1020 Could you issue a PR?

@lemire
Copy link
Member

lemire commented Dec 22, 2020

This has been fixed.

@lemire lemire closed this as completed Dec 22, 2020
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

No branches or pull requests

3 participants