Closed RubyNova closed 1 year ago
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>)
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.)
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?
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.
After having a quick look, i couldn't see any uses of GSL features outside gsl::span
.
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.
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 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
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.
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 ?
Based on conversation in Discord , this proposal has been Approved under the following specification:
std::span
usage via CMake / Source buildsstd::span
usage is not officially supported by the NovelRT teamNow open to be picked up 😄
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.
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 introducesstd::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