-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Splitting out pybind11/stl/filesystem.h. #3077
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
Changes from 1 commit
7b8c8f4
9365a3a
3e6b76a
bc84589
b0f60fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // Copyright (c) 2021 The Pybind Development Team. | ||
| // All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| #pragma once | ||
|
|
||
| #include "../detail/common.h" | ||
|
|
||
| #ifdef __has_include | ||
| # if defined(PYBIND11_CPP17) && __has_include(<filesystem>) && \ | ||
| PY_VERSION_HEX >= 0x03060000 | ||
| # include <filesystem> | ||
| # define PYBIND11_HAS_FILESYSTEM 1 | ||
| # endif | ||
| #endif | ||
|
|
||
| PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) | ||
| PYBIND11_NAMESPACE_BEGIN(detail) | ||
|
|
||
| #if defined(PYBIND11_HAS_FILESYSTEM) | ||
| template<typename T> struct path_caster { | ||
|
|
||
| private: | ||
| static PyObject* unicode_from_fs_native(const std::string& w) { | ||
| #if !defined(PYPY_VERSION) | ||
| return PyUnicode_DecodeFSDefaultAndSize(w.c_str(), ssize_t(w.size())); | ||
| #else | ||
| // PyPy mistakenly declares the first parameter as non-const. | ||
| return PyUnicode_DecodeFSDefaultAndSize( | ||
| const_cast<char*>(w.c_str()), ssize_t(w.size())); | ||
| #endif | ||
| } | ||
|
|
||
| static PyObject* unicode_from_fs_native(const std::wstring& w) { | ||
| return PyUnicode_FromWideChar(w.c_str(), ssize_t(w.size())); | ||
| } | ||
|
|
||
| public: | ||
| static handle cast(const T& path, return_value_policy, handle) { | ||
| if (auto py_str = unicode_from_fs_native(path.native())) { | ||
| return module::import("pathlib").attr("Path")(reinterpret_steal<object>(py_str)) | ||
| .release(); | ||
| } | ||
| return nullptr; | ||
| } | ||
|
|
||
| bool load(handle handle, bool) { | ||
| // PyUnicode_FSConverter and PyUnicode_FSDecoder normally take care of | ||
| // calling PyOS_FSPath themselves, but that's broken on PyPy (PyPy | ||
| // issue #3168) so we do it ourselves instead. | ||
| PyObject* buf = PyOS_FSPath(handle.ptr()); | ||
| if (!buf) { | ||
| PyErr_Clear(); | ||
| return false; | ||
| } | ||
| PyObject* native = nullptr; | ||
| if constexpr (std::is_same_v<typename T::value_type, char>) { | ||
| if (PyUnicode_FSConverter(buf, &native)) { | ||
| if (auto c_str = PyBytes_AsString(native)) { | ||
| // AsString returns a pointer to the internal buffer, which | ||
| // must not be free'd. | ||
| value = c_str; | ||
| } | ||
| } | ||
| } else if constexpr (std::is_same_v<typename T::value_type, wchar_t>) { | ||
| if (PyUnicode_FSDecoder(buf, &native)) { | ||
| if (auto c_str = PyUnicode_AsWideCharString(native, nullptr)) { | ||
| // AsWideCharString returns a new string that must be free'd. | ||
| value = c_str; // Copies the string. | ||
| PyMem_Free(c_str); | ||
| } | ||
| } | ||
| } | ||
| Py_XDECREF(native); | ||
| Py_DECREF(buf); | ||
| if (PyErr_Occurred()) { | ||
| PyErr_Clear(); | ||
| return false; | ||
| } else { | ||
|
||
| return true; | ||
| } | ||
| } | ||
|
|
||
| PYBIND11_TYPE_CASTER(T, _("os.PathLike")); | ||
| }; | ||
|
|
||
| template<> struct type_caster<std::filesystem::path> | ||
| : public path_caster<std::filesystem::path> {}; | ||
| #endif // PYBIND11_HAS_FILESYSTEM | ||
|
|
||
| PYBIND11_NAMESPACE_END(detail) | ||
| PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, do we need this
PYBIND11_HAS_FILESYSTEMnow? If I include<stl/filesystem.h>do I want it to silently not cast things instead of getting a compile-time error?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If not, it should be moved to the include in the test ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying my question from the slack channel here:
Ideally we'd give people a choice? Bail out with a compile-time error or have them handle the runtime error.
It's not really great if we lead users down the path of copy-pasting the
__has_includelogic from the test.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But do users want "works if available" behavior for this? (The answer may be yes) It seems a little weird if your package only accepts
pathlib.Patharguments if it was compiled in C++17 mode on some compilers. And only allows you to return std::filesystem on Python 3 (though that I'm less worried about). I'm guessing there will be some extra work most users will have to do to make sure this works (including requiring macOS 10.15+), so forcing the user to explicit might be better.That is, I expect most users will either make an effort to support std::filesystem/pathlib everywhere, or they will not support it, but very few will support it if available. But could easily be wrong. I don't find the "auto" selection of C++ level all that useful either (setuptools helpers), but some users want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay to leave it as is, a user (who looks at the source) can check
PYBIND11_HAS_FILESYSTEMif they want. But I'm also okay to (re)move it to the tests.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__has_includeis not actually a C++17 feature, it's a C++20 one. Though I think all major compilers added it very early, but still, maybe it's best to remove this and just#includefilesystem and let the failure happen if it's not available, rather than calling#error. This then makesPYBIND11_HAS_FILESYSTEMrather weird, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@henryiii This page suggest it is a C++17 feature though? https://en.cppreference.com/w/cpp/preprocessor/include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
official spec: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0061r1.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__has_includecurrently appears in 10 source code locations on master (see below).If think for this PR it's important to stay focused on the one problem we want to solve as soon as possible: users shouldn't be forced to work on the link options if they don't actually want to use the new caster. I'll update the PR description accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @Skylion007 is correct, I was muddling has_include with feature test macros - probably shouldn't comment on things while in a conference. :)
This plans seems fine, we need to fix this, then we can polish and or document a bit more later if needed.