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

Include <filesystem> only if FMT_CPP_LIB_FILESYSTEM is set #4258

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

W4RH4WK
Copy link
Contributor

@W4RH4WK W4RH4WK commented Dec 7, 2024

This change results out of necessity since the Nintendo Switch console SDK does not support std::filesystem. The SDK still provides the <filesystem> header, but with an #error directive, effectively breaking any build that includes <filesystem>

Because <filesystem> is present, FMT_HAS_INCLUDE is insufficient here. With this change and FMT_CPP_LIB_FILESYSTEM in place, one can define FMT_CPP_LIB_FILESYSTEM=0 to work around this issue.

This assumes that <filesystem> can be included (without warnings) if FMT_CPP_LIB_FILESYSTEM is set. If this is not the case, fmt would be broken even before this change as std::filesystem::path is used without the accompanying header.


I've tested this locally using:

  • MSVC Version 19.42.34435 (Win10)
  • GCC 12.2.0 (WSL)
  • Clang 15.0.6 (WSL)
  • Clang Nintendo version 1.16.4 (Clang 16.0.6)

This change results out of necessity since the Nintendo Switch console
SDK does not support `std::filesystem`. The SDK still provides the
`<filesystem>` header, but with an `#error` directive, effectively
breaking any build that includes `<filesystem>`

Because `<filesystem>` is present, `FMT_HAS_INCLUDE` is insufficient
here. With this change and `FMT_CPP_LIB_FILESYSTEM` in place, one can
define `FMT_CPP_LIB_FILESYSTEM=0` to work around this issue.

This assumes that `<filesystem>` can be included (without warnings) if
`FMT_CPP_LIB_FILESYSTEM` is set. If this is not the case, fmt would be
broken even before this change as `std::filesystem::path` is used
without the accompanying header.
@phprus
Copy link
Contributor

phprus commented Dec 7, 2024

This PR is break stantart compatible implementations, because __cpp_lib_filesystem is defined in <filesystem>.

https://en.cppreference.com/w/cpp/utility/feature_test

@vitaut
Copy link
Contributor

vitaut commented Dec 7, 2024

Feature-test macros are also defined in <version> so it should be fine.

@vitaut vitaut merged commit 9600fee into fmtlib:master Dec 7, 2024
45 checks passed
@vitaut
Copy link
Contributor

vitaut commented Dec 7, 2024

Merged, thanks!

@phprus
Copy link
Contributor

phprus commented Dec 7, 2024

Feature-test macros are also defined in <version> so it should be fine.

Only in C++20, but <filesystem> is C++17.

@vitaut
Copy link
Contributor

vitaut commented Dec 7, 2024

Hmm, I guess we could define FMT_CPP_LIB_FILESYSTEM to nonzero if FMT_CPLUSPLUS >= 201703L and FMT_HAS_INCLUDE(<filesystem>) is true to handle this case.

@phprus
Copy link
Contributor

phprus commented Dec 7, 2024

Hmm, I guess we could define FMT_CPP_LIB_FILESYSTEM to nonzero if FMT_CPLUSPLUS >= 201703L and FMT_HAS_INCLUDE(<filesystem>) is true to handle this case.

Fix: #4259

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.

3 participants