Move to c++17 while keeping old boost #8355
Conversation
* tests don't compile because of gtest's assumptions * hack around features removed in C++17 for boost used in kiva * remove std::binary_function references from alphanum
* These utilities are removed in C++17 (only msvc enforces this) * They are not actually used by anything, just ther by convention
* MacOS CI is currently breaking, it makes me wonder if it's picking up two different versions of boost * We'll see if this fixes it
lefticus
added
the
Refactoring
|
Well, those results are certainly looking great! I'll take a look at the changes, and may kick off a package build on Github Actions just to make sure it is also happy with building this. |
There was a problem hiding this comment.
This looks fantastic. I wish I could filter out files by path and skip looking at the gtest changes, but so be it. I scrolled to the bottom and found the minor change to Kiva's cmake list. I don't actually have any problem with this. Like I said, I'm going to be kicking off a package build on Github Actions of this branch just to make sure it is happy building there. As long as it is happy this looks good and should be able to go in. If @mbadams5 is able to check out the change that would be cool too though. Thanks @lefticus !
|
Github Actions had no issues building package from this branch, so I am going to say I am satisfied with it. I'll still hold until tomorrow morning to let @mbadams5 or anyone else have a look if they want, but I suspect it is good to go. |
|
Here we go then, merging. Thanks @lefticus! |
|
@lefticus I want to clarify if there are actual changes that still need to happen in libkiva. I would love to get rid of the boost dependency (mostly to get rid of its bloat). I will likely need to any make corresponding changes to the official Kiva repository. |
|
@nealkruis I had to hack up and around Boost 1.61.0 to get it compiling in C++17 mode on MacOS with libc++ and Windows MSVC2019. When discussing this with a boost expert I know he asked why we were using such an old boost. So I tried to update boost and then all of the geometry calculations were incorrect. TL;DR updating boost is probably a really good idea if you're going to keep it in the project, but it breaks something I don't understand if we do update it. But we are moving forward with C++17 right now even without the boost update. |
|
@lefticus Thanks for the explanation! It's good to confirm that nothing is needed immediately. I will look into this separately. |
This enabled C++17 mode throughout EnergyPlus
Notes and caveats:
BOOST_NO_AUTO_PTRflag because boost is not properly detecting the version of Clang on MacOSNote to @nealkruis we really need to update the version of Boost included with libkiva. It is very old, and caused most of the pain here. Unfortunately if we upgrade Boost then all of the geometry becomes wrong inside of libkiva.
To be clear, we still compile with no warnings or errors with Boost::Geometry, we simply get the wrong answers now. And specifically we get the wrong area and perimeter calculations.
I investigated the issues and found that the order of points in the geometry are now incorrect with a newer version of boost. I can find no documentation that states why this would be the case. But with the order of points now different all of the calculations change.
Otherwise the PR should get us moving into C++17 land!