microsoft / GSL

Guidelines Support Library
Other
6.13k stars 737 forks source link

compilation errors around string_span #1070

Closed beinhaerter closed 1 year ago

beinhaerter commented 1 year ago

I just tried to switch part of my code from span<const char> to cstring_span (bound to C++14, so std::string_view is not yet available to me). With this code

#include <gsl/string_span>
gsl::cstring_span x;

I got some compiler errors. I fixed them by adding

#include "byte"     // for byte
#include "span"     // for calculate_byte_size

to gsl/string_span, but only to get the final error

my.cpp(56,7): error C4996: 'gsl::cstring_span': string_span was removed from the C++ Core Guidelines. For more information, see isocpp/CppCoreGuidelines PR#1680 my.cpp(56,20): error C2641: cannot deduce template arguments for 'gsl::cstring_span'

My concern is that the error is only triggered after fixing missing includes. This is not helpful. In my opinion we should either a) remove string_span completely or b) strip it down to a version that does not require additional includes and that directly emits the desired error. Do you prefer a) or b)? I am willing to prepare a PR.

dmitrykobets-msft commented 1 year ago

Hi @beinhaerter,

I agree, string_span has been deprecated for a while now and can be removed except for the *zstring family definitions (basic_zstring, czstring, ...), so option (b) is my preference. Note, though, that there may be tests that need to be removed and/or ported to use gsl::span, that were previously using string_span. If the test porting is too big of a task to include here, that can be done later, since those tests are not testing supported functionality anyways.

Thanks, Dmitry

beinhaerter commented 1 year ago

Hi Dmitry,

when talking about string_span I was not precise enough, I meant class string_span, not file string_span. From what you write it looks like we agree on removing *class*string_spanand keeping the rest of *file*string_span`. I'll open a PR for that change.

Cheers Werner