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

A new namespace was introduced in 11.1.0 fmt_detail #4324

Open
odygrd opened this issue Jan 23, 2025 · 3 comments
Open

A new namespace was introduced in 11.1.0 fmt_detail #4324

odygrd opened this issue Jan 23, 2025 · 3 comments

Comments

@odygrd
Copy link

odygrd commented Jan 23, 2025

I believe code should be consistent and use the existing fmt::detail namespace. There is a new namespace that was introduced in latest version fmt_detail. Would it be possible to use the existing fmt::detail or a fmt::detail::chrono namespace ?

namespace fmt_detail {

For example when bundling libfmt under a custom namespace with a script, that is now an additional namespace that needs to be renamed

@odygrd odygrd changed the title A new namespace was introduced in 11.1.0 fmt_detail A new namespace was introduced in 11.1.0 fmt_detail Jan 23, 2025
@vitaut
Copy link
Contributor

vitaut commented Jan 25, 2025

We can't use fmt or fmt::detail namespace there but a subnamespace of fmt::detail could potentially work.

@odygrd
Copy link
Author

odygrd commented Jan 26, 2025

I made a PR with the simplest change I could think:

#4328

@odygrd
Copy link
Author

odygrd commented Jan 29, 2025

It looks like when current_zone is placed in a fmt::fmt_detail nested namespace—while also having using namespace fmt_detail;—unqualified lookup finds the template version first within fmt_detail, preventing it from considering std::chrono's function.

On the other hand, if the user-defined templated current_zone function is in a completely separate namespace, unqualified lookup doesn't find it before std::chrono's non-template version.

https://godbolt.org/z/49Tr89Pej

I also tried the following approach, completely removing the fmt_detail namespace:

template <class, class = void> struct has_tzset : std::false_type {};

template <class T>
struct has_tzset<T, void_t<decltype(_tzset())>> : std::true_type {};

template <typename, typename = void>
struct has_current_zone : std::false_type {};

template <typename T>
struct has_current_zone<T, void_t<decltype(std::chrono::current_zone())>>
    : std::true_type {};

Attempting to call std::chrono::current_zone() via SFINAE in a pre-C++20 environment fails due to the decltype check. Adding preprocessor checks like #if defined(__cpp_lib_chrono) && __cpp_lib_chrono >= 201907L and #if defined(_GLIBCXX_USE_CXX11_ABI) would likely resolve this.

I think that if someone tries to use two different versions of libfmt simultaneously in the future—like fmt v12 and fmt v11—they might run into the same collision issue with the current fmt_detail namespace, though I’m not sure how common that scenario would be.

Feel free to close this issue, I think it's not worth the effort to change. Just wanted to share my findings in case they’re useful.

Thanks again for the amazing work on this library! 🚀

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

No branches or pull requests

2 participants