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

gsl/narrow should include <exception> #1044

Merged
merged 1 commit into from Apr 26, 2022

Conversation

TheJCAB
Copy link
Member

@TheJCAB TheJCAB commented Apr 25, 2022

This file uses std::exception, so it should include the appropriate header.

Normally it gets the STL's "exception" header included via the gsl/assert file, but this is skipped with _HAS_EXCEPTIONS=0. I understand _HAS_EXCEPTIONS is undocumented and unsupported, but regardless, the appropriate header should be included here.

Alternatively, gsl/narrow should be modified to support _HAS_EXCEPTIONS=0, like gsl/assert was. But I'm not proposing that change. "exception" does define std::exception even with _HAS_EXCEPTIONS=0.

This file uses std::exception, so it should include the appropriate header.

Normally it gets the STL's <exception> header included via the gsl/assert file, but this is skipped with _HAS_EXCEPTIONS=0. I understand _HAS_EXCEPTIONS is undocumented and unsupported, but regardless, the appropriate header should be included here.

Alternatively, gsl/narrow should be modified to support _HAS_EXCEPTIONS=0, like gsl/assert was. But I'm not proposing that change. <exception> does define std::exception even with _HAS_EXCEPTIONS=0.
@dmitrykobets-msft
Copy link
Member

Hi @TheJCAB, thanks for helping clean up the code here, the fix looks good.
I think you are missing a word in the last sentence of your change description. Could you please fix that? Thanks.

@TheJCAB
Copy link
Member Author

TheJCAB commented Apr 26, 2022

@dmitrykobets-msft "missing a word in the last sentence of your change description" - It was complete, but I guess angle brackets are not good here. Too used to ADO's markup, I suppose. Thanx! Fixed.

@dmitrykobets-msft dmitrykobets-msft merged commit f21f29d into microsoft:main Apr 26, 2022
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

2 participants