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

shielding #789 behind a new #define to effectively disable it now #802

Merged
merged 1 commit into from Jan 16, 2018

Conversation

Projects
None yet
3 participants
@eddelbuettel
Member

eddelbuettel commented Jan 15, 2018

No description provided.

@kevinushey

This comment has been minimized.

Contributor

kevinushey commented on 81c8339 Jan 15, 2018

Can't we just have Rcpp_eval_impl() always use Rf_eval() for now, rather than sprinkling the #ifdefs around the associated usage? I think that would imply just ensuring that RCPP_USE_PROTECT_UNWIND is never defined by us?

This comment has been minimized.

Contributor

kevinushey replied Jan 15, 2018

(I think this looks fine but I think we can accomplish the same thing with a bit less code)

This comment has been minimized.

Member

eddelbuettel replied Jan 15, 2018

Sure, we could. So far I figured @lionel- 's PR was somewhat limited so I could just leave it in place, "protect" it (or us ;-) ) from it and give him a chance to revisit in time.

These ifdef will not stay forever. Either we can make this work, or I'll remove them in, say, six month. At least now I know where to find the code as the #ifdef provide a clear demarcation.

This comment has been minimized.

Contributor

lionel- replied Jan 15, 2018

What I don't understand is that Rcpp_eval_impl() was defined as a simple wrapper to Rf_eval() unless RCPP_PROTECTED_EVAL was defined..

This comment has been minimized.

Member

eddelbuettel replied Jan 15, 2018

See the diff. Your PR affected other areas, and things generally turned sour ... on systems other than Linux. I spent two days cleaning this and am not too interested in discussions of the "in theory this should have worked" type because I have ample evidence that "in practice" this didn't. Part of it may be R. We don't quite know.

But now is neither the time to take chance, nor for experimentation. Let's get a working 0,12,15 out the door.

@eddelbuettel eddelbuettel merged commit 7a9122a into master Jan 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@eddelbuettel eddelbuettel deleted the feature/condition_unwind_protect branch Jan 21, 2018

@eddelbuettel eddelbuettel restored the feature/condition_unwind_protect branch Jul 23, 2018

@eddelbuettel eddelbuettel deleted the feature/condition_unwind_protect branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment