Skip to content

Conversation

iam
Copy link
Contributor

@iam iam commented Aug 11, 2018

Shall we continue our discussion on how we can upstream this now that I have rebased everything to tip-of-tree ?

100% tests passed, 0 tests failed out of 67

Total Test time (real) =   9.03 sec

iam added 2 commits August 10, 2018 17:39
Change-Id: Id623455d32e9323355744a240c2813d0411d1dac
std::exception_ptr usage is replaced with rxcpp::util::error_ptr
which will typedef to std::exception_ptr when exceptions are enabled.

When exceptions are disabled this will typedef to an internal error
type that can retain the "what" error message.

Additionally std::current_exception() and similar usages are replaced
with rxu::current_exception which uses error_ptr instead.

Lastly all try/catch/throw keywords are replaced with either
RXCPP_TRY, RXCPP_CATCH, rxu::throw_exception or similar.

Note that try/catch/throw keywords cause a compilation error with
-fno-exceptions. Trying to access most of the std::*exception* functions
will call std::terminate at runtime.

Tests using exceptions must be disabled by passing --nothrow to the
check2 test runner.

Change-Id: I0b95ae2e323653a17c3b733d165ecf87a014c315
@kirkshoop
Copy link
Member

looks great! the CI builds seem to need some minor changes. vc will need [[noreturn]] to be macroized. the doxygen examples have a few minor issues.

@iam
Copy link
Contributor Author

iam commented Aug 18, 2018

That sounds straightforward enough to fix.

Clarifying questions:

  • Did you want me to plumb through the fno-except/fno-rtti up to the CMakeFiles somehow as an extra flag?
  • What about adding presubmit support for this new mode?

(Note that I am unfamiliar with CMake best practices, so I would be glad to hear of a specific suggestion if you have one in mind).

@kirkshoop
Copy link
Member

Good points. Regressions will be much less likely is there is a cmake option and a ci pass for this. I’ll add something for those if you don’t get to it first. :)

I am certainly not a cmake proficient but I did recently add some cmake options to change compiler flags in the facebookresearch/pushmi repo that could be a start.

@kirkshoop kirkshoop merged commit 4aa52e4 into ReactiveX:master Oct 27, 2018
@kirkshoop
Copy link
Member

Thank you so much for this @iam!

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.

2 participants