Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

operator[] checks whether given index is valid. #889

Closed
aforster opened this issue May 25, 2020 · 6 comments
Closed

operator[] checks whether given index is valid. #889

aforster opened this issue May 25, 2020 · 6 comments

Comments

@aforster
Copy link

I was interested in the reasoning for this bound check here and why that it is not guarded by #ifdef NDEBUG:

https://github.com/microsoft/GSL/blob/master/include/gsl/span#L613

This can be quite a big performance hit. Are the maintainers interested in a pull request that changes the Expects/Ensures macros when NDEBUG is defined?

@grahamreeds
Copy link

grahamreeds commented May 25, 2020 via email

@unixod
Copy link

unixod commented May 28, 2020

Hi,
Alternatively to NDEBUG I suggest to consider also #877 where I proposed to return back the configuration options for Expects/Ensures, which existed before.

@JordanMaples
Copy link
Contributor

The intended behavior of gsl::span is to guarantee bounds safety in all access operations, which is why we intentionally do not wrap these checks in NDEBUG.

@aforster
Copy link
Author

Ok, I guess if this is by design I will close the issue.

@ericLemanissier
Copy link
Contributor

well, C++ standards says that it's a precondition that the index is valid https://eel.is/c++draft/span.elem#1
If gsl::span has different assumptions that std::span, it would be wise to use another name for the class, to avoid misleading your users

@JordanMaples
Copy link
Contributor

Maintainer's call: We actually are following the span design on purpose, however the only difference is the bounds checking. The intent is to be able to simply using a different namespace and it will still work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants