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

Move from C++14 plus extensions to C++17 and refine cmake configuration #9

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rkaminsk
Copy link
Member

@rkaminsk rkaminsk commented Jul 9, 2021

This PR switches to C++17 and refines the cmake configuration. The motivation was to make plasp compile on MacOS (which failed badly). Like this, it should in principle even compile on Windows, too. I would have switched to C++17's variant too but this would have required to change all the match functions. I also removed extra options from the cmake configuration files. Those are of course nice for development but are guaranteed to cause friction when someone tries to package the project. They could be added back but should not be enabled by default.

I have seen (too late) that the project's development branch is quite a bit ahead of the master. In any case, it should not be hard to merge back changes into the development branch.

I have also seen that your were preparing to switch to meson. I would vote not to do this. Cmake has admittedly worst case syntax plus some other flaws but it is one of the most widely used build systems. I used it in a lot of our projects and it is quite easy to painlessly support a wide range of platforms. Also, with the recent changes the configuration does not even look that ugly.

Feel free to discard this PR if you want to take another route.

EDIT: There are also some small changes like missing includes and additional overloads to support compilation on MacOS (size_t is defined as unsigned long long).

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.

1 participant