Support VS2019 16.3 and higher

[aholzinger]

VS2019 since 16.3 has deprecated std::experimental::filesystem with #error in the corresponding header making the compikation fail.

With this small patch it's working:

--- a/nmos-cpp/Development/bst/filesystem.h Thu Jan 16 14:07:34 2020
+++ b/nmos-cpp/Development/bst/filesystem.h Thu Jan 16 14:07:36 2020
@@ -17,6 +17,9 @@

 #if _MSC_VER >= 1900
 #define BST_FILESYSTEM_STD_EXPERIMENTAL
+#if _MSC_VER >= 1920
+#define _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING
+#endif
 #else
 #define BST_FILESYSTEM_MICROSOFT_TR2
 #endif

By moving to C++17 instead filesystem would be in std::filesystem, but I guess VS2015 is the blocker for this change.

[garethsb]

Thanks, I'll investigate.

[garethsb]

It may be possible to use std::filesystem in this case, via BST_FILESYSTEM_STD, something like this:

--- a/nmos-cpp/Development/bst/filesystem.h
+++ b/nmos-cpp/Development/bst/filesystem.h
@@ -17,6 +17,8 @@

+#if _MSC_VER >= 1920
+#define BST_FILESYSTEM_STD
+#elif _MSC_VER >= 1900
-#if _MSC_VER >= 1900
 #define BST_FILESYSTEM_STD_EXPERIMENTAL
 #else
 #define BST_FILESYSTEM_MICROSOFT_TR2
 #endif

[aholzinger]

Unfortunately not. Only if you set the language standard to C++17 otherwise std::filesystem is not defined.

[garethsb]

OK, understood, thanks. I guess ultimately then we should support both modes, depending on value of __cplusplus preprocessor symbol, based on /Zc:__cplusplus compiler option.

[aholzinger]

Yes, that would be the best solution, but I guess that would mean to support choosing C++17 (or also C++14?) for the whole project, wouldn't it?

On GitHub there is a FindFileSystem CMake extension of vector-of-bool that handles finding the right filesystem implementation: https://github.com/vector-of-bool/CMakeCM/blob/master/modules/FindFilesystem.cmake

[garethsb]

Thanks, Axel. This project needs to support C++11 compilers/standard library, but it is intended to also compile with a C++17 compiler/library. Both (meta)build and source changes may be required to do that. The bst namespace is the mechanism to prevent source-code changes being required in the rest of the codebase as much as possible. (The 'bst' mechanism was more relevant when used to compile with C++98/03 compilers where many of the eventual C++11 libraries could be emulated by aliasing Boost libraries into the bst namespace instead of the C++11 std libraries.)

[aholzinger]

Got it. I think in this case for VS2019 (16.3 and newer) unfortunately the #define _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING will be needed and filesystem will remain std::experimental::filesystem. I think this is perfectly okay.

And perhaps CMake in one of the next releases will come up with a FindFilesystem solution already integrated in CMake.

[garethsb]

So in my understanding, compilation with VS2019 16.3 using <experimental/filesystem> doesn't fail, just produces the warning, which (nmos-cpp or) the user can suppress using _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING? Or the user could start using /std:c++17 and nmos-cpp could detect this using __cplusplus or maybe _HAS_CPP17 or feature detection macros as in https://stackoverflow.com/a/53365539/12250441.

Since the current situation doesn't result in a compilation failure, for now, I will leave the definition of the preprocessor symbol to suppress the warning up to the user.

[aholzinger]

Unfortunately it's not a warning that is produced, it's an #error, so I think setting _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING is the right thing to do.

nmos-cpp sets /std:c++11 in CMakeLists.txt (or one of the included *.cmake files). How would one 'overrule' this to set /std:c++17?

Using C++17 with VS2019 I think is fine, I just don't know how to acheive this and additionaly with the current bst/filesystem.h it's not possible, as with C++17 it has to be std::filesystem instead of std::experimental::filesystem.

[garethsb]

Unfortunately it's not a warning that is produced, it's an #error, so I think setting _SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING is the right thing to do.

Ha, OK, I understand! Thank you, Microsoft, for the misleading preprocessor symbol name. 😄

Also CMake doesn't make it especially easy for users to add preprocessor symbols themselves, so I will use the patch you originally suggested, thank you!

nmos-cpp sets /std:c++11 in CMakeLists.txt (or one of the included *.cmake files). How would one 'overrule' this to set /std:c++17? Using C++17 with VS2019 I think is fine, I just don't know how to acheive this

That's true. I hope this is as simple as changing the line here: https://github.com/sony/nmos-cpp/blob/master/Development/cmake/NmosCppCommon.cmake#L14 to:

set(CMAKE_CXX_STANDARD 11 CACHE STRING "Default value for CXX_STANDARD property of targets")

and additionaly with the current bst/filesystem.h it's not possible, as with C++17 it has to be std::filesystem instead of std::experimental::filesystem.

It is possible with current bst/filesystem.h by setting BST_FILESYSTEM_STD before including it, but as mentioned, CMake doesn't make this as easy as we'd like. Hence why I proposed modifying bst/filesystem.h to check __cplusplus or maybe _HAS_CPP17 or use feature detection macros as in https://stackoverflow.com/a/53365539/12250441 to do this automatically for VS2019 in /std:c++17 mode.

[garethsb]

@aholzinger I've pushed something simple that should allow you to experiment with (a) /std:c++17 (though of course there may be other reasons this doesn't work!) and std::filesystem, and (b) sticking with default language version and std::experimental::filesystem but avoid the #error.