On 13/10/21 21:19 +0100, Jonathan Wakely wrote:
>On 13/10/21 20:41 +0100, Jonathan Wakely wrote:
>>Adjust the __detail::__effective_range overloads so they always return a
>>string or string view using std::char_traits, because we don't care
>>about the traits of an incoming string.
>>
>>Use std::contiguous_iterator in the __effective_range(const Source&)
>>overload, to allow returning a basic_string_view in more cases. For the
>>non-contiguous cases in both __effective_range and __string_from_range,
>>return a std::string instead of std::u8string when the value type of the
>>range is char8_t.  These changes avoid unnecessary basic_string
>>temporaries.
>
>[...]
>
>>  template<typename _InputIterator>
>>    inline auto
>>    __string_from_range(_InputIterator __first, _InputIterator __last)
>>    {
>>      using _EcharT
>>	= typename std::iterator_traits<_InputIterator>::value_type;
>>-      static_assert(__is_encoded_char<_EcharT>);
>>+      static_assert(__is_encoded_char<_EcharT>); // C++17 [fs.req]/3
>>
>>-#if __cpp_lib_concepts
>>-      constexpr bool __contiguous = std::contiguous_iterator<_InputIterator>;
>>-#else
>>-      constexpr bool __contiguous
>>-	= is_pointer_v<decltype(std::__niter_base(__first))>;
>>-#endif
>>-      if constexpr (__contiguous)
>>+      if constexpr (__is_contiguous<_InputIterator>)
>
>Oops, this pessimiszes construction from string::iterator and
>vector::iterator in C++17 mode, because the new __is_contiguous
>variable template just uses is_pointer_v, without the __niter_base
>call that unwraps a __normal_iterator.
>
>That means that we now create a basic_string<C> temporary where we
>previously jsut returned a basic_string_view<C>.
>
>I am testing a fix.

Here's the fix, committed to trunk.

With this change there are no temporaries for string and vector
iterators passed to path constructors. For debug mode there are some
unnecessary temporaries for vector::iterator arguments, because the
safe iterator isn't recognized as contiguous in C++17 mode (but it's
fine in C++20 mode).

The bytes allocated to construct a path consisting of a single
filename with 38 characters are:

Constructor arguments               GCC 11 | GCC 12 | 11 debug | 12 debug
-------------------------------------------|--------|----------|---------
const char*                           39   |   39   |    39    |   39
const char(&)[N]                      39   |   39   |    39    |   39
const char8_t*                        39   |   39   |    39    |   39
const char8_t(&)[N]                   39   |   39   |    39    |   39
std::string::iterator pair            39   |   39   |    39    |   39
std::string::iterator NTCTS           92   |   39   |    92    |   39
std::u8string::iterator pair          39   |   39   |    39    |   39
std::u8string::iterator NTCTS        131   |   39   |   131    |   39
std::vector<char8_t>::iterator pair   39   |   39   |    78    |   78
std::vector<char8_t>::iterator NTCTS 131   |   39   |   131    |   39
std::list<char8_t>::iterator pair     78   |   78   |    78    |   78
std::list<char8_t>::iterator NTCTS   131   |  131   |   131    |  131

So for GCC 12 there are no unwanted allocations unless the iterators
are not contiguous (e.g. std::list::iterator). In that case we need to
construct a temporary string. For the pair(Iter, Iter) constructor we
can use distance(first, last) to size that temporary string correctly,
but for the path(const Source&) constructor we read one character at a
time from the input and call push_back.

In any case, the regression is fixed and we're at least as good as GCC
11 in all cases now.