microsoft / GSL

Guidelines Support Library
Other
6.11k stars 736 forks source link

Type mismatch in span::operator[] #1110

Closed jdgarciauc3m closed 1 year ago

jdgarciauc3m commented 1 year ago

The interface for span_iterator::operator[] uses difference_type (which is compatible with gsl::index).

However span::operator[] uses size_type. This causes type mismatches and is not compatible with some strict warning modes.

Perhaps the interface should be unified?

dmitrykobets-msft commented 1 year ago

Hi @jdgarciauc3m, could you please provide a code snippet which raises the warning you are seeing?

jdgarciauc3m commented 1 year ago

Sure!

See the following piece of code

  std::array v {1, 2, 3};
  gsl::span t{v};
  gsl::index i = 1;
  auto x = t[i];
  std::cout << x << '\n';

When compiled with clang++-15 and the following flags: -Wall -Wextra -Weffc++ -Werror -pedantic -pedantic-errors -Wconversion, the following error is obtained:

[build] error: implicit conversion changes signedness: 'gsl::index' (aka 'long') to 'gsl::span::size_type' (aka 'unsigned long') [-Werror,-Wsign-conversion]
[build]   auto x = t[i];
[build]            ~ ^
[build] 1 error generated.

Similar error is obtained with gcc-12 and -Wsign-conversion.

dmitrykobets-msft commented 1 year ago

Thanks for the repro. The warning that appears is not about the span's iterator's index operator, but rather about the mismatch in types between gsl::index (which is signed), and the span's operator[] parameter (which is unsigned), in the expression auto x = t[i];

The choice of types appears to be by design. ES.107: Don’t use unsigned for subscripts, prefer gsl::index encourages the use of a signed index type for safety reasons, but points out that the C++ standard is still using unsigned indices. This other discussion reaffirms the design decision behind wanting signed indices. At one point, gsl::span's operator[] did indeed accept a signed parameter, but this was changed in order to conform with std::span. If I replace gsl::span with std::span in your code example I get the same warning. Unfortunately, while the standard containers continue to accept unsigned indices, the use of gsl::index will trigger conversion warnings if they are enabled globally by default, hence the pessimistic enforcement section under ES.107.

As for your question regarding the mismatch between span's and its iterator's operator[], I'll need to look a bit deeper first before getting back to you.

dmitrykobets-msft commented 1 year ago

Update, regarding the difference between span's and its iterator's operator[] parameter:

This is again, by design. A span does not have elements prior to the 0 index, whereas its iterator can point anywhere in its range, meaning negative indexing is perfectly valid. The following code will raise the same warning you were seeing before, except now complaining about converting from an unsigned value to a signed value:

  std::array v {1, 2, 3};
  gsl::span t{v};
  size_t i = 1;
  auto x = t.begin()[i]; // implicit conversion changes signedness: 'size_t' (aka 'unsigned long') to 'gsl::details::span_iterator::difference_type' (aka 'long') [-Werror,-Wsign-conversion]
  std::cout << x << '\n';

The same is true for std::span.