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

Different Rcout object for each translation unit #928

Closed
Enchufa2 opened this issue Dec 3, 2018 · 16 comments
Closed

Different Rcout object for each translation unit #928

Enchufa2 opened this issue Dec 3, 2018 · 16 comments

Comments

@Enchufa2
Copy link
Member

Enchufa2 commented Dec 3, 2018

Opening this as per request of @kevinushey. As explained in this thread,

Rcout is defined in iostream/Rstreambuf.h as a static object. This means that different translation units see a different Rcout, while they see the same std::cout (I have a minimal package showing this if needed). As a result, for example, one object allocated in a certain cpp cannot redirect the output of other object allocated in another cpp.

And the same for Rcerr. I think that this might be worth fixing on the Rcpp side, but probably it would require to declare Rcout and Rcerr as extern and generate some code to initialise them in RcppExports.cpp.

@github-actions
Copy link

This issue is stale (365 days without activity) and will be closed in 31 days unless new activity is seen. Please feel free to re-open it is still a concern, possibly with additional data.

@Enchufa2
Copy link
Member Author

I think this is still worth fixing.

@eddelbuettel
Copy link
Member

Sure. Any chance you could take a stab at it in branch?

@Enchufa2
Copy link
Member Author

Yes, I would like to. I'll try to find some time for this in the coming weeks. My approach would be to follow the design of std::cout if there is no complication that I'm not aware of.

Enchufa2 added a commit to Enchufa2/Rcpp that referenced this issue Jan 28, 2021
@Enchufa2
Copy link
Member Author

Enchufa2 commented Jan 28, 2021

There's a first pass at this here: Enchufa2@d96d806. I would like to ensure that this is the right approach before submitting a PR. Basically I have:

  • declared Rcout and Rcerr (previously static objects) as extern references;
  • defined two routines, Rcpp_cout_get and Rcpp_cerr_get, that create static objects and return a reference to them.

Now the problem is that the initialization of such references must be part of the scaffolding code. For example, in RcppExports.cpp, in calls to cppFunction, sourceCpp... So far, I've managed to make this work:

f <- "CharacterVector ptr%s() {
  std::ostringstream address;
  address << (void const *)(&%s);
  return address.str();
}"

Rcpp::cppFunction(sprintf(f, "A", "Rcout"))
Rcpp::cppFunction(sprintf(f, "B", "Rcout"))
ptrA() == ptrB() # should be TRUE

So:

  • Is this the best approach?
  • If so, could you please help me identify all the places in which the initialization code should be injected? (Ideally, there should be one place to put this, but I'm not sure whether this is possible without a heavy refactoring).

@Enchufa2
Copy link
Member Author

Enchufa2 commented Jan 28, 2021

BTW, a motivating example that could be used for testing:

Rcpp::cppFunction('void to_left() { Rcout << std::left; }')
Rcpp::cppFunction('void something() { Rcout << std::setw(20) << "something" << std::endl; }')
something() # right-aligned by default
to_left() # change alignment
something() # should be left-aligned

This is the kind of thing that bit me when I opened this, but in the context of two different .cpp files in a package. It took a while to find out what was happening, because there's a single stdout, a single std::cout, and similarly one expects a single Rcpp::Rcout.

@eddelbuettel
Copy link
Member

Right. Rcout support was a reasonably early contribution that hugely helpful in coordinating with the R i/o streams. It hasn't really been revisited since. Switching, possibly optionally, to this new scheme would be nice. I guess you are in a man-to-man combat with the Attributes code here combined for good measure with the non-trivial initialization code.

@Enchufa2
Copy link
Member Author

By some quick trial and error, I managed to get this pass all the tests except for the use of Rcout in the context of sourcing a file with sourceCpp. I need more study of that part of the code.

@eddelbuettel
Copy link
Member

Never mind. That comment was meant for our Twitter DM stream. Redacted here.

@kevinushey
Copy link
Contributor

I'm a little worried that re-defining Rcout / Rcerr in this way could be brittle, in that it could be easy to miss places where Rcout / Rcerr should have been initialized, but weren't.

What do you think about instead defining Rcout() and Rcerr() functions, that return a reference to a static object living in the Rcpp library? So one could do:

Rcout() << do << the << thing;

and get the expected result.

Enchufa2 added a commit to Enchufa2/Rcpp that referenced this issue Jan 28, 2021
@Enchufa2
Copy link
Member Author

Give #1139 an opportunity, please. :) The advantage is that all the code out there will work just by setting a flag.

@eddelbuettel
Copy link
Member

The rev.dep machine has already been turned on :)

@eddelbuettel
Copy link
Member

No new issues, as we expected given that an #ifdef shields things: RcppCore/rcpp-logs@fffe2d3

@Enchufa2
Copy link
Member Author

I wouldn't expect any issue with the new define set either.

@eddelbuettel
Copy link
Member

Right. Which gets us to a next point: could you possibly cook up an example or better still a unit test?

eddelbuettel added a commit that referenced this issue Feb 1, 2021
Address #928: enable global Rcout/Rcerr via RCPP_USE_GLOBAL_ROSTREAM
@eddelbuettel
Copy link
Member

(The PR had a "tag" for it but no "closes #928" so doing this by hand, with a tip-of-the-hat and Thank You! for the 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

No branches or pull requests

3 participants