Switch to C++17

[jbytheway]

Summary

Infrastructure "Convert build to C++17 (from C++14)"

Purpose of change

Following up on #63970 and #64011.

We want to move to C++17 to get new features and generally keep the codebase up to date.

Describe the solution

Update each of the build files to use C++17 (Makefile, CMakeLists.txt, Visual Studio projects).

Fix the various compiler errors and clang-tidy warnings that arise.

Describe alternatives you've considered

Waiting until #64011 was already merged. It is merged now.

Testing

Unit tests, clang-tidy.

Mostly this needs to be tested on CI, so I have to open it as a non-draft, but marking it WIP because there's bound to be issues on other compilers.

Additional context

[irwiss]

Considering not all cpp17 features can be used; can structured binding be used? Asking both from compiler support and code style perspectives? E.g.

nutrients this_min;
nutrients this_max;
std::tie( this_min, this_max ) = compute_nutrient_range( result_it, rec, extra_flags );

into
auto [this_min, this_max] = compute_nutrient_range(...
(as you can see type information isn't visible)
Decomposing pairs is by far the most annoying part of using them

[jbytheway]

From a compiler support perspective, yes, structured bindings are fine.

From a code style perspective, I think we will probably allow them in certain situations. But since we do have the "almost always avoid auto" rule, we'll probably want to be cautious about how we adopt them. Only time (and @kevingranade) will tell.

At the least I hope to use them for iterating over containers of pairs, which is a super common pattern in our codebase.

[jbytheway]

For reference, the C++17 features which we should avoid are documented here,

[BrettDong]

Maybe revert back to ghc file system and see if permission errors go away?

[jbytheway]

Maybe revert back to ghc file system and see if permission errors go away?

@akrieger is hoping to test locally and see what can be done.

[kevingranade]

I saw an instance of this in an unrelated PR yesterday and it resolved with a rerun, so at a minimum I don't think this PR is the cause.

[jbytheway]

Good to know, thanks!

[RAldrich]

I would appreciate it if we could resolve #64210 before merging this. (Not going to fix / dropping Debian support is totally a possible resolution - I just think it would be best to make a decision before merging additional compiler changes).

[jbytheway]

I think the issue in #64210 should be mostly orthogonal to this, but thanks for the heads up.

[akrieger]

Same failure again, seems suspect. I scrolled through other recent failures and didn't see the same.

I tested yesterday and didn't repro, but realized just now I tested with 2022, so I'm going to revert back to 2019 and try again today.

[BrettDong]

Successfully reproduced the error. Built with VC2019 build tools.

[BrettDong]

The error is caused by the trailing / in the path requested. I made it working in VC2019 by removing trailing slash:

diff --git a/src/filesystem.cpp b/src/filesystem.cpp
index e498da3736..d437f50470 100644
--- a/src/filesystem.cpp
+++ b/src/filesystem.cpp
@@ -21,7 +21,11 @@

 bool assure_dir_exist( const std::string &path )
 {
-    return assure_dir_exist( fs::u8path( path ) );
+    std::string p = path;
+    if (!p.empty() && p.back() == '/') {
+        p.pop_back();
+    }
+    return assure_dir_exist( fs::u8path( p ));
 }

 bool assure_dir_exist( const cata_path &path )

[akrieger]

[edit: apparently not this bug in the VS patch notes] I saw something related to that in the some 2019 patch notes. I believe there are many fixes listed in https://learn.microsoft.com/en-us/cpp/overview/cpp-conformance-improvements-2019?view=msvc-170 which are backports of recent standards committee meetings, including stuff about trailing slashes like mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87846

[akrieger]

Specifically https://cplusplus.github.io/LWG/issue3096 is probably the relevant change.

[jbytheway]

Thanks for the diagnosis. I've pushed a tentative fix.

[kevingranade]

Regarding the item density test, I thought it was something like that too, but it turns out it's much more prosaic, the clang++-12 workflow is the first one that runs with magiclysm loaded, and you were hitting a magiclysm-specific data error that was fixed in #64258
Other than that, it looks like this will be good to go with a rebase.

[jbytheway]

OK, I've removed the item density fix and rebased on master.

Based on previous experience on this PR, the CI is going to take a long time to run (presumably because all the cached data is useless here).

[akrieger]

A lot of this lint shouldve been caught pre-17, no?

[jbytheway]

I think this is good to go, so I've removed the WIP label.