-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Address #928: enable global Rcout/Rcerr via RCPP_USE_GLOBAL_ROSTREAM #1139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks pretty clean. I put two minor comments / questions in.
Looking into those unit tests. |
I'll take a look later -- thanks a bunch for cooking up a test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me but I'll probably let it sit here for a moment to let @kevinushey pipe in too.
The test is nice work, by the way. Apart from the gawdawful two space indenting, of course 😉 |
Ok, I'll wait til 'end of day' in whatever suitable time zone to see if @kevinushey has a comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! LGTM.
I think I got it. I've identified two places in
attributes.cpp
for the initialization that I believe cover all the cases. The new behaviour is enabled for Rcpp, but disabled by default for Rcpp users, guarded byRCPP_USE_GLOBAL_ROSTREAM
. All tests pass with and without setting this in~/.R/Makevars
.Checklist
R CMD check
still passes all tests