Proposal: Add new type alias - `NovelRT::Span<T>`

[RubyNova]

21 Jan 2023 - Approved as per comment

What is the current behaviour?
Currently we are directly using gsl::span<T>, which does work, but C++20 introduces std::span<T>, which our codebase just won't be using once we switch over.

What is the expected behaviour/change?
By adding this new type alias, we can support both scenarios.

What is the motivation / use case for changing the behavior?
By changing over our usage of gsl::span<T> to a type alias that changes based on language version,

Describe alternatives you've considered:
The only real alternative is to just swap everything over to std::span<T> when the time comes, but that will stop C++17 from working, which has been mentioned to me as some people possibly still wanting that.

Are there any potential roadblocks or challenges facing this change?
No obstalces known at this time - its should just be a preprocessor define or two.

Are there any downsides to implementing this change?
Having to double check C++17 is still working when we migrate will be a pain.

Additional context
N/A

[Pheubel]

After looking at https://github.com/microsoft/GSL/wiki/gsl::span-and-std::span and the source code, I don't think there will be any noticeable differences besides GSL having runtime bounds checking.

And having to double check C++17 alongside C++ 20 seems to be inevitable with the engine taking more advantage of being compiled with newer language features.

I think that a mock-up with power to the end user to switch between GSL and STD spans would look something like this:

#if !defined( NOVELRT_SPAN_FORCE_GSL ) || __has_include(<span>)
#include <span>
#if __cpp_lib_span
using NrtSpan = std::span;
#else
using NrtSpan = gls::span;
#endif //__cpp_lib_span

#else
using NrtSpan = gls::span;
#endif //!defined( NOVELRT_SPAN_FORCE_GSL ) || __has_include(<span>)

[capnkenny]

We should probably introduce it as a proper type (NovelRT::Span vs NrtSpan), but if this can be a global change (and everything else switched to the NovelRT-specific span) then I think this would work.

Realistically, although there's not much supposed difference I am still one of giving choice to the end user - however the following should be looked into as well:

• Are there other usages of GSL besides gsl::span? If not, then do we opt to not include GSL at all when it's unnecessary?
  (Specifically in the use case that NovelRT is shipping with C++20's span support.)

• Do we need to use the "force GSL" flag when compiling with C++17 support?
  If not, do we support its usage in the following scenario? -> C++20 compilation, but forcing GSL usage.
  (If not, we'll want to be clear that the flag's usage in that scenario will not be supported by the maintainers.)

[RubyNova]

If we make it as an actual type we will need to make sure expose at least enough of the API so that we can easily use it. The reason a typedef was suggested is because it basically skips this step.

We can probably cut GSL from C++20 builds.

Can you clarify what you mean by "use the force GSL flag" here? Do you mean in @Pheubel 's example?

[capnkenny]

Soooo...

1. We should be able to typedef it under the NovelRT namespace, no? My memory is foggy, but if both versions expose the same API, then throwing it under the NovelRT root namespace shouldn't be terrible. Maybe I'm wrong though.
2. And yes, I do mean like in @Pheubel 's example - specifically in the case that a C++20 compiler is provided that flag to use GSL.

[Pheubel]

After having a quick look, i couldn't see any uses of GSL features outside gsl::span.

[RubyNova]

We should be able to typedef it under the NovelRT namespace, no? My memory is foggy, but if both versions expose the same API, then throwing it under the NovelRT root namespace shouldn't be terrible. Maybe I'm wrong though.

Right that's the idea.

And yes, I do mean like in @Pheubel 's example - specifically in the case that a C++20 compiler is provided that flag to use GSL.

I think the force flag is only required if the span header does not exist in Kat's example. I don't know how reliable it is across all 3 compilers though.

[capnkenny]

Okay, so it sounds like we only have the concern about __has_include and how the compiler will react with this.

Based on what I've read, there's three scenarios here:

1. It just works (LOL)
2. It doesn't work consistently, and there will be false positives/negatives
3. We can use that check, but possibly implement a compiler check in CMake to do this prior to building dependencies
   (The thought process here is that if we check that the compiler includes , we can toggle a flag and react accordingly since we'll know both a) the C++ standard we're compiling with, and b) if the headers are properly implemented).

1 just worries me because we just assume it works so that's out.
2 is plausable, but opens up an issue when we start introducing other compilers...
Does anyone have an issue with option 3? @Pheubel / @RubyNova

[RubyNova]

We could just use a switch everywhere then detect in CMake to turn it on or not based on language version. That's what I'd probably do. At least we know what'll happen in that situation.

[capnkenny]

Okay. Then in that case, as long as the impl. follows what's been proposed here, I don't mind approving this.
Any concerns before I switch this to being an open issue for pickup @RubyNova ?

[capnkenny]

Based on conversation in Discord , this proposal has been Approved under the following specification:

• GSL will continue to be the accepted behaviour in official use cases
• We will allow end-users to opt-in to std::span usage via CMake / Source builds
• If opted in, it will be noted that std::span usage is not officially supported by the NovelRT team
• README / Wiki updates to follow suit

[capnkenny]

Now open to be picked up 😄

[Pheubel]

I think that implementing it into the code base is a task I could finish quickly, as we've already had quite the conversation about it on discord. The wiki/readme part will most likely not be as quick and might be better left off to someone else.