feature: support compilers that use std::experimental::filesystem #3840
Conversation
for more information, see https://pre-commit.ci
…uder/pybind11 into support-experimental-filesystem
for more information, see https://pre-commit.ci
include/pybind11/stl/filesystem.h
Outdated
| # elif __has_include(<experimental/filesystem>) | ||
| # include <experimental/filesystem> | ||
| # define PYBIND11_HAS_EXPERIMENTAL_FILESYSTEM 1 | ||
| namespace pybind11_filesystem_alias = std::experimental::filesystem; |
There was a problem hiding this comment.
Actually, any reason it needs to / should be in the global namespace? Do we want people downstream to reuse it? Even if we do wouldn't be better to just make it part of the pybind11 namespace?
There was a problem hiding this comment.
I agree, totally doesn't need to be global
There was a problem hiding this comment.
I'm fine either way, I tried to keep it simple for you.
As long as there is a pybind11 prefix, I don't think we'll ever clash with anyone outside.
Formally putting it in pybind11::detail would seem ideal, but at first sight it seems slightly tricky, especially because there is macro logic around the pybind11 namespace that I don't understand 100% TBH.
There was a problem hiding this comment.
Yeah, I would try sticking it in detail and see if anything blows up.
There was a problem hiding this comment.
On second glance, I can't actually think of any way to completely remove this from the public API here. I can put it under the pybind11 namespace if that makes us feel better. Between pybind11_filesystem_alias or pybind11::filesystem_alias, I think the intent is clear enough. They're both unlikely to conflict with any user namespaces, they both have clear intent, and even if you were to use it, the can't cause any real damage; it's just a namespace alias.
EDIT: I should have hit refresh before posting. This is all largely redundant now. I'll see what I can do about fitting this into detail
There was a problem hiding this comment.
@Llcoolsouder the detail namespace is how we denote private APIs / functions in pybind11. Putting under the pybind11 namespace would still be better though than leaving it in the current namespace like how it currently is.
There was a problem hiding this comment.
@Skylion007 so we would move the macro logic to detail and then use pybind11::detail::filesystem_alias instead of the current pybind11_filesystem_alias? Or am I missing something?
There was a problem hiding this comment.
So if I move this to detail, I have to #include <filesystem> (or #include <experimental/filesystem>) in detail in order to use the namespace alias. So filesystem would be included in every file that includes detail/common.h.
Thoughts?
(Personally at this point, I would just opt for pybind11::filesystem_alias in stl/filesystem.h)
| # error \ | ||
| "#include <filesystem> is not available. (Use -DPYBIND11_HAS_FILESYSTEM_IS_OPTIONAL to ignore.)" | ||
| "Neither #include <filesystem> nor #include <experimental/filesystem is available. (Use -DPYBIND11_HAS_FILESYSTEM_IS_OPTIONAL to ignore.)" | ||
| #endif | ||
|
|
||
| PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) | ||
| PYBIND11_NAMESPACE_BEGIN(detail) |
There was a problem hiding this comment.
detail namespace begins here. Can you not just move the include logic after line 38?
There was a problem hiding this comment.
If not, it's fine as is.
There was a problem hiding this comment.
Why do we even need the namespace btw? It's only used on the type caster right? Just mimic what we do for experimental optional and spell it out twice with an IFDEF?
There was a problem hiding this comment.
Addressed in the latest commit.
Since we're here though, what is the point of the PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END macros? It looks like we're just replacing the language syntax with a lesser known syntax.
There was a problem hiding this comment.
From memory: in some environments the macros insert "visibility hidden" attributes.
for more information, see https://pre-commit.ci
feature: support compilers that use std::experimental::filesystem such as gcc7
Description
Minor changes to support older C++ compilers where filesystem is not yet part of the standard library and is instead included in std::experimental::filesystem.
Suggested changelog entry:
Minor changes to support older C++ compilers where filesystem is not yet part of the standard library and is instead included in ``std::experimental::filesystem``.