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

Various library layout updates #3

Closed
wants to merge 5 commits into from
Closed

Conversation

flippmoke
Copy link
Contributor

@flippmoke flippmoke commented Sep 6, 2018

Includes the following updates:

  • Now a library based around mapbox hpp skel
  • Made into true header only library (removed src directory)
  • Added google benchmark
  • Followed good C++ development guidelines
    • Removed global using of std namespace
    • Used std::size_t and std::int64_t as necessary
    • Remove various unrequired includes
  • Updated to use mason packaging system instead of script for setup
  • Added more travis tests
  • New Commands via Makefile
    • make or make 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

@flippmoke
Copy link
Contributor Author

I should also note that I did not run make tidy or make format yet. Both are likely to change quite a few things for the better though. I would suggest trying them locally and seeing if you like the changes first.

@flippmoke
Copy link
Contributor Author

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.

@delfrrr
Copy link
Owner

delfrrr commented Sep 7, 2018

@flippmoke wow, thanks for PR! It's both amazing and overwhelming 😉; I will need some time to process it.

A brief look on changes:
👍 header only library
👍 google benchmark
👍 std:: and proper size types (need to check as at some places unsigned introduces inconsistent behavior with JS version)

Not very clear to me:
🤔 what is motivation for using mason

I suggest moving into separate PRs/issues (unless it's absolutely necessarily for you to use it)
.clang-* and formating/validation
⏳ coverage check

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!

@flippmoke
Copy link
Contributor Author

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 -fsanitizer options in clang within travis.

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 .clang-format file but you are welcome to change it to your liking.

@delfrrr
Copy link
Owner

delfrrr commented Sep 8, 2018

I see, it's all about adopting hpp-skel. which is a goof thing itself.
It still does not build on Travis. I see at least one problem: root Makefile is missing.

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.

@delfrrr delfrrr mentioned this pull request Sep 9, 2018
@delfrrr
Copy link
Owner

delfrrr commented Sep 9, 2018

Moving to #6

@delfrrr delfrrr closed this Sep 9, 2018
@delfrrr delfrrr mentioned this pull request Sep 9, 2018
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.

2 participants