Skip to content

Enable usage of gsl::narrow with exceptions disabled#640

Merged
annagrin merged 3 commits intomicrosoft:masterfrom
annagrin:dev/annagrin/no_exception_support
Mar 15, 2018
Merged

Enable usage of gsl::narrow with exceptions disabled#640
annagrin merged 3 commits intomicrosoft:masterfrom
annagrin:dev/annagrin/no_exception_support

Conversation

@annagrin
Copy link

This change is based on the previous PR by @djarek:

This solution uses the approach of boost::asio to enabling usage of the
library in environments where exception usage is either prohibited
or not feasible (due to code size constraints). A function template
gsl::throw_exception has been added, which in a normal environment just
throws the exception. However, when GSL_TERMINATE_ON_CONTRACT_VIOLATION
is defined the function is only declared by gsl and the definition of
this function template must be supplied by the library's user.

Closes: #468

Signed-off-by: Damian Jarek damian.jarek93@gmail.com

Addition:

  • made sure narrow() terminates only in GSL_TERMINATE_ON_CONTRACT_VIOLATION
    mode, and throws in all others
  • added work around for MS STL to ensure termination in no-exception mode
    - understand MSVC STL no exception macro
    - use function static variable to set termination handler in no-exception mode (msvc)
  • add compile-only tests for no-exception mode

// Temporary until MSVC STL supports no-exceptions mode.
// Currently terminate is a no-op in this mode, so we add termination behavior back
//
#if defined(_MSC_VER) && !_HAS_EXCEPTIONS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to test if _HAS_EXCEPTIONS is #defined before testing its value. People catch me making that mistake all the time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

#include <gsl/gsl_util> // for narrow_cast, at
#include <gsl/span> // for span, span_iterator, operator==, operator!=

#include "test_noexcept.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere else we use .h I think. Any reason to not follow suit here and have test_noexcept.h?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from catch.hpp, but no reason to do it here

///////////////////////////////////////////////////////////////////////////////

#include "test_noexcept.hpp"
int main()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't you want to install a replacement to the usual "terminate" behavior in this test so you can ensure each case ends up at the terminate-handler as expected without having to actually kill the process?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not using the catch infrastructure, need to think about the best way to implement this, so will resolve it in a later PR

@neilmacintosh
Copy link
Contributor

I really like the change overall, thanks for picking up @djarek's work here @annagrin and finishing it up. I left a couple of comments to consider but (apart from the #if-related one) you should feel free to merge and get to them at your leisure.

@annagrin annagrin force-pushed the dev/annagrin/no_exception_support branch from 7d8d1a7 to a14aeef Compare March 13, 2018 20:56
@annagrin
Copy link
Author

Addressed #if-related and .hpp comments. Will implement testing for no-exception mode separately.

@neilmacintosh
Copy link
Contributor

LGTM, thanks @annagrin!

djarek and others added 3 commits March 15, 2018 14:42
This solution uses the approach of boost::asio to enabling usage of the
library in environments where exception usage is either prohibited
or not feasible (due to code size constraints). A function template
gsl::throw_exception has been added, which in a normal environment just
throws the exception. However, when GSL_TERMINATE_ON_CONTRACT_VIOLATION
is defined the function is only declared by gsl and the definition of
this function template must be supplied by the library's user.

Closes: microsoft#468

Signed-off-by: Damian Jarek <damian.jarek93@gmail.com>

Addition:
- understand STL no exception macro
- use function static variable to set termination handler in kernel mode
- add compile-only tests for no-exception mode
@annagrin annagrin force-pushed the dev/annagrin/no_exception_support branch from f9236a7 to 4c49c9e Compare March 15, 2018 21:43
@annagrin annagrin merged commit d846fe5 into microsoft:master Mar 15, 2018
@annagrin
Copy link
Author

this closes #468 and #635

@annagrin annagrin deleted the dev/annagrin/no_exception_support branch March 16, 2018 01:09
@beinhaerter
Copy link
Contributor

With this latest change I get a warning from VS 15.6.2 core guideline code analysis (I'ld prefer not to see warnings from GSL):

gsl\include\gsl\gsl_assert(106): warning C26440: Function 'gsl::details::throw_exceptiongsl::narrowing_error' can be declared 'noexcept' (f.6: http://go.microsoft.com/fwlink/?linkid=853927).

We could add noexcept but then other warnings come up ("gsl::narrow can be declared noexcept"). So I suggest suppressing the warning. What do you think? Shall I prepare a PR?

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.

4 participants