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

Avoiding infinite recursion with path_view #32

Closed
vitaut opened this issue Aug 21, 2019 · 7 comments
Closed

Avoiding infinite recursion with path_view #32

vitaut opened this issue Aug 21, 2019 · 7 comments

Comments

@vitaut
Copy link

vitaut commented Aug 21, 2019

I've stumbled upon an interesting problem when working with std::filesystem::path. Consider the following example (https://godbolt.org/z/wDoxgT):

#include <filesystem>
#include <iostream>

template< class T >
concept Range = requires(T&& t) {
  std::begin(std::forward<T>(t));
  std::end  (std::forward<T>(t));
};

template <typename T>
void print(const T& value) {
  std::cout << value;
}

template <Range R>
void print(const R& range) {
  for (const auto& elem: range) {
    print(elem);
  }
}

int main() {
  print(std::filesystem::path("foo"));
}

Running it gives stack overflow because a single-element path is a range that contains itself as an element which causes infinite recursion. Unfortunately we cannot solve this problem in std::filesystem::path but would it be possible for path_view to do better, for example, by splitting path_view into two types such as path_view and path_view_component with the latter having all features of the former but not being a range?

@vitaut
Copy link
Author

vitaut commented Aug 21, 2019

Here's a real-world example of this problem with std::filesystem::path: fmtlib/fmt#1268.

@vitaut
Copy link
Author

vitaut commented Aug 21, 2019

AFAICS the current path_view implementation doesn't have iterator:

// const_iterator
so this is more of a feedback for P1030R2 std::filesystem::path_view.

@ned14
Copy link
Owner

ned14 commented Aug 22, 2019

May I post your example to WG21 mailing reflector, to gain feedback on what is best to do with path_view?

Note that there is no iterator support in the implementation because I had to go get feedback from SG16 Unicode on how best to do it. But I'm literally writing the new path_view right now.

@vitaut
Copy link
Author

vitaut commented Aug 22, 2019

May I post your example to WG21 mailing reflector, to gain feedback on what is best to do with path_view?

Sure.

Another interesting data point is that from my Twitter poll, > 40% of people think that the example will work (print something), which is worrying.

@ned14
Copy link
Owner

ned14 commented Aug 22, 2019

I would agree with them, it should work.

While I have you as the author of {fmt}, can I persuade you to add support for raw gather buffer formatting to {fmt}? Then one can do cool stuff like:

llfio::file_handle &fh;
fh.write(offset, fmt::out("System error code = {}\n", errno));

Here out(...) simply emits an iterable which iterates something smelling like span<const byte>.

I appreciate your print() i/o function et al, but we can go much lower level without programmer inconvenience. Plus out() works immediately with Networking, LLFIO, anything which consumes gather buffers etc, no extra work.

@ned14
Copy link
Owner

ned14 commented Aug 23, 2019

Given the lack of useful feedback from WG21 mailing reflector, I've gone ahead and implemented your proposed path_view_component workaround. You can see it in the wip branch, which doesn't compile yet. Thanks for raising this issue, it hadn't occurred to me before, and I'm glad to be able to try fixing it.

@vitaut
Copy link
Author

vitaut commented Aug 23, 2019

While I have you as the author of {fmt}, can I persuade you to add support for raw gather buffer formatting to {fmt}?

I added your suggestion as an issue (fmtlib/fmt#1271) and will look into it.

I've gone ahead and implemented your proposed path_view_component workaround.

Awesome, thanks!

@vitaut vitaut closed this as completed Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants