Comparison operator on string_spans - null-terminated and otherwise. about gsl-lite

[martinmoene]

Started to implement basic_string_span.

[martinmoene]

Hi Magnus (@Sillamacka),

I've added basic_string_span in branch basic_string_span. I both looked at the proposal p0123r2 and M-GSL.

Can you check if the changes fix the failures you had?

Thanks,
Martin

[martinmoene]

Thanks Magnus,

Summary:

• rbegin() & rend() should return reverse_iterator
• [Note: split into two functions as with span]
  Signed/unsigned problems in basic_string_span::subspan().
• Default constructor for basic_string_span was marked noexcept, but default constructor for span was not.
• Changed the std::basic_string const &-taking constructor to work with non-default char_traits and allocator template parameters.
• [AIUI the workaround (and the way M-GSL goes about it) is 'just fine']
  And finally, when compiling with Clang-3.9, basic_string_span's move & copy constructor SFINAE causes errors when instantiating span::span(span const &) within the is_convertible check. I guess the overload should be disabled with SFINAE instead of duck typing in the body?
  Curiously, GCC-6.2.0 handles it just fine. I wonder which is correct? (Both are compiled with the same version of libstdc++.)
  As a workaround, I replaced the is_convertible test to check if the pointer types are convertible instead.

See below for:

• [Had noticed this, will look into it]
  Another point: I think it would be useful to allow operator== on directly on string_spans and types which are convertible to string_span. M-GSL does this, although it does not appear to be in the proposal.

[Magnutic]

Another, simpler, approach would be in construction, i.e. ignore the null-terminator when constructing from string literal. This would cover most cases where the comparison gives unexpected results while keeping the current behaviour of the comparisons.

[martinmoene]

Thanks Magnus,

I'll have a look at the construction-case.

Perhaps time has come to add class basic_string_span instead of building string_span on on span?

[Magnutic]

Nice! I too started thinking that ignoring the null terminator on construction is better.
With my additional comparison operator experiment, there are now four versions of the comparisons, which seems a bit too much - besides, a user might want to explicitly include the null terminator in the span and its comparisons.

[Magnutic]

Hello, that was fast!

It did fix the problems, but I had to fix some bugs in the code first: here is the diff.
Summary: rbegin() & rend() should return reverse_iterator
Signed/unsigned problems in basic_string_span::subspan().
Default constructor for basic_string_span was marked noexcept, but default constructor for span was not.

And finally, when compiling with Clang-3.9, basic_string_span's move & copy constructor SFINAE causes errors when instantiating span<char>::span(span<const char> const &) within the is_convertible check. I guess the overload should be disabled with SFINAE instead of duck typing in the body?

Curiously, GCC-6.2.0 handles it just fine. I wonder which is correct? (Both are compiled with the same version of libstdc++.)

As a workaround, I replaced the is_convertible test to check if the pointer types are convertible instead.

Another point: I think it would be useful to allow operator== on directly on string_spans and types which are convertible to string_span. M-GSL does this, although it does not appear to be in the proposal.

Thank you for your work!

[Magnutic]

It seems the strange Clang error is triggered when calling gsl::to_string() on any string_span.
Minimal example:

#include "gsl-lite.h"
#include <string>

int main() {
    gsl::cstring_span span = "Hello world";
    std::string str = gsl::to_string(span);
}

I can also confirm that it can be solved by constraining span<T>::span( span<U> const & ) with SFINAE.

[Magnutic]

Excellent! I forgot to mention that I also changed the std::basic_string const &-taking constructor to work with non-default char_traits and allocator template parameters, perhaps you should add that to the list?

[martinmoene]

I'd noticed it though :)

[martinmoene]

@Sillamacka , I'll first release gsl lite with basic_string_span without its enhanced comparison as indicated below, then have a look at it:

• [Had noticed this, will look into it]
  Another point: I think it would be useful to allow operator== on directly on string_spans and types which are convertible to string_span. M-GSL does this, although it does not appear to be in the proposal.

[Magnutic]

Sounds good to me, explicitly constructing cstring_spans in comparisons isn't that much extra work, so I think I can deal with it :D

[martinmoene]

It'll not take long probably...

[martinmoene]

Published release 0.16.0 with enhanced string span comparison.

[Magnutic]

Great! I think we can consider this issue resolved now.