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

Changed implementation of gsl::narrow to throw gsl::narrowing_error #873

Merged
merged 5 commits into from
Apr 23, 2020

Conversation

londey
Copy link
Contributor

@londey londey commented Apr 22, 2020

Related to issue #872

Reinstated throwing behaviour of gsl::narrow described in C++ Core Guidlines ES.46

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es46-avoid-lossy-narrowing-truncating-arithmetic-conversions

Implementation now behaves as described in the C++ Core Guidlines
@JordanMaples
Copy link
Contributor

JordanMaples commented Apr 22, 2020

I took a look at the build issues you're having.

Deleting the file no_exceptions_throw_test.cpp and removing the related line in tests/CMakeLists.txt will get everything building cleanly.

That file exists because of the old terminate vs throw behavior and is no longer needed.

@londey
Copy link
Contributor Author

londey commented Apr 23, 2020

@JordanMaples #873 (comment) I wasn't sure if you wanted to keep the no-throw option for narrow and there doesn't seem to be a define to control if GSL should be no-throw.

If you are happy to remove that scenario I can remove the test.

@JordanMaples JordanMaples merged commit 601b55e into microsoft:master Apr 23, 2020
@JordanMaples
Copy link
Contributor

Thanks again for finding this issue and getting the fix ready.

@beinhaerter
Copy link
Contributor

@JordanMaples I was thinking about upgrading to GSL 3.0. Seeing this fix here (and using gsl::narrow in my code) I now know that I cannot use GSL 3.0. Do you plan to release a new version of GSL like 3.1 with this fix here included?

@JordanMaples
Copy link
Contributor

@beinhaerter My original plan was to release 3.1.0 when the span paper P1976R2 was implemented, but I think this warrants a minor release. I'll be pushing 3.0.1 today.

@JordanMaples
Copy link
Contributor

@beinhaerter I've released 3.0.1.

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

Successfully merging this pull request may close these issues.

None yet

3 participants