search

SFTtech / openage

Free (as in freedom) open source clone of the Age of Empires II engine 🚀
http://openage.dev
Other
12.73k stars 1.12k forks source link

Updating headers with std::filesystem for Win #1230

Open simonsan opened 4 years ago

simonsan commented 4 years ago

There were some problems in the past regarding std::filesystem, MSVC 2017 and 2019 added support so we would be able to update our sources correspondingly.

std::filesystem for VS2017

TheJJ commented 4 years ago

I would recommend to replace our custom path handling within libopenage/util/fslike/directory.h to use std::filesystem instead of our custom implementation. The frontent, our path and filesystem abstraction (openage/util/fslike/ and libopenage/util/fslike/) should stay and then be a wrapper around std::filesystem.

This is because our path system supports passing paths between C++ and Python, and std::filesystem can only be used for native C++ paths.

sl1g18 commented 4 years ago

I would recommend to replace our custom path handling within libopenage/util/fslike/directory.h to use std::filesystem instead of our custom implementation. The frontent, our path and filesystem abstraction (openage/util/fslike/ and libopenage/util/fslike/) should stay and then be a wrapper around std::filesystem.

This is because our path system supports passing paths between C++ and Python, and std::filesystem can only be used for native C++ paths.

I think with a bit of support I'll be able to tackle this one as my first contribution :)

simonsan commented 4 years ago

I think with a bit of support I'll be able to tackle this one as my first contribution :)

Nice, and for sure we will be able to support ;-) As a starter you could read a small workflow recommendation by us so here: https://github.com/SFTtech/openage/blob/master/doc/contributing.md#workflow

For example just by making your Draft-PR early and marking it [WIP] we will be able to look easily into things, you and we can comment on lines of code with questions, feedback or explanations etc.

sandsmark commented 4 years ago

heads up, though, is that std::filesystem is (or at least was) very crashy-crashy with msvc so I'd suggest a lot of testing before merging anything.

iirc I ended up blaming some internal refcounting in their path objects, it seemed to go away once I switched to always convert to strings in freeaoe (instead of storing std::filesystem::paths). aaand remember generic_string() vs string().