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.

Sign up for GitHub

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

Jump to bottom

Make it possible to use as header only library #24

Merged
merged 1 commit into from
Sep 27, 2016
Merged

Make it possible to use as header only library #24

merged 1 commit into from
Sep 27, 2016

Conversation

tmpsantos
Copy link
Contributor

@tmpsantos tmpsantos commented Sep 23, 2016 •
edited

It is still possible to use it as a static library.

@tmpsantos
Copy link
Contributor Author

👀 @kkaefer

@tmpsantos tmpsantos changed the title Make it possible to build as header only library Make it possible to use as header only library Sep 23, 2016
@tmpsantos
Copy link
Contributor Author

The reason why I'm pushing this is because geojson-cpp is currently the only C++ static library dependency of mapbox-gl-native core and has been a major source of headache regarding CXX11ABI. Making it optionally header only would eliminate the problem if you already depend on rapidjson.

mapbox/mason#251 (comment)

Secondly this will help me with the mapbox-gl-native Windows 10 build I've been playing with. :)

@tmpsantos tmpsantos changed the title Make it possible to use as header only library [DO NOT MERGE] Make it possible to use as header only library Sep 23, 2016
kkaefer
kkaefer requested changes Sep 26, 2016
View reviewed changes
include/mapbox/geojson.hpp

#if !defined(MAPBOX_GEOJSON_LIBRARY)
#include "geojson_impl.hpp"
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not do this in a header file, but rather in a separate *.cpp file to avoid issues with having geojson.hpp included more than once, which leads to duplicate definitions. So instead of including the implementation here, we should do it in src/mapbox/geojson.cpp.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also means that we don't need the define flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kkaefer, this makes a lot more sense.

@tmpsantos
Copy link
Contributor Author

New patch for review, I tested this all the way to mapbox-gl-native and it works.

@tmpsantos tmpsantos changed the title [DO NOT MERGE] Make it possible to use as header only library Make it possible to use as header only library Sep 26, 2016
@tmpsantos tmpsantos self-assigned this Sep 26, 2016
@tmpsantos tmpsantos merged commit 6506ea5 into master Sep 27, 2016
@tmpsantos tmpsantos deleted the header_only branch September 27, 2016 08:33
This was referenced Oct 17, 2016
Closed
Open
@kkaefer kkaefer mentioned this pull request Nov 22, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkaefer kkaefer kkaefer approved these changes

Assignees

@tmpsantos tmpsantos

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tmpsantos @kkaefer