-
Notifications
You must be signed in to change notification settings - Fork 100
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
Various library layout updates #3
Conversation
…google benchmark, fixed various configuration issues, moved the namespace some
I should also note that I did not run |
I forgot to add that this is a great little library and I can help explain any of the decisions of this rather large pull request. |
@flippmoke wow, thanks for PR! It's both amazing and overwhelming 😉; I will need some time to process it. A brief look on changes: Not very clear to me: I suggest moving into separate PRs/issues (unless it's absolutely necessarily for you to use it) In general, let's keep everything you need to start using the library and leave everything else for later. I have a strong preference for introducing changes and setup complexity more gradually 🙏 Other suggested changes are relevant, please create issues for them. And finally we need to make CI build work again 😉 Appreciate a lot you contribution! |
mason has been useful because it allows for Mapbox to prebuild some binaries and libraries at pinned versions during development. This greatly speeds up our CI process because it allows for things like google benchmark (which we link against here) to be pulled down as a local copy. The idea is that every library pulls all of its dependencies from a local version rather then perhaps developing with multiple versions of different developers locally. The binaries that are prebuilt in mason are typically built for linux/android/mac/ios right away so you can just start building instead of worrying about configuration. Another example of this in the pull request is that the travis job will pull in different prebuilt versions of compilers if desired for testing. This allows consistent testing of things such as building with The use of clang tidy and clang format are both part of hpp-skel so by default I just included them. Clang tidy is very useful for finding bugs and hidden issues for us as it does some static analysis. Clang format typically is very nice to make consistent formatting, we have a suggested format in the |
I see, it's all about adopting hpp-skel. which is a goof thing itself. If you don't mind, I will do couple of changes on top of your's to fix build and maybe simplify couple of things. |
Moving to #6 |
Includes the following updates:
src
directory)std
namespacestd::size_t
andstd::int64_t
as necessarymake
ormake debug
make test
make bench
make format
make tidy
make coverage
Other changes suggested but not implemented yet:
The
Delaunator
struct (previously a class) is a bit of a mess as most of the work is done in the constructor. The many of the member variables are not likely needed outside the constructor either as they do not have a lifetime beyond that. Overall, I would suggest just making this a single function that calls other functions and returns the triangle geometries rather then using a class or struct at all.The accepted data format for entry of points is currently a
std::vector<double>
where each point is a pair of doubles. Instead it might be far easier to either used a templated geometry object such as something like geometry.hpp and its multipoint object. If each point is a struct inside your input vector this allows for much easier index manipulation.The changes from @morishuz in #1 are not included here.
/cc @mourner