Skip to content

provide full messaging system - #206

Open
paciorek wants to merge 10 commits into
mainfrom
messaging
Open

provide full messaging system#206
paciorek wants to merge 10 commits into
mainfrom
messaging

Conversation

@paciorek

Copy link
Copy Markdown
Contributor

This surfaces the work I did some time ago on a careful, comprehensive messaging system.

We decided not to try to merge this in for now.

When we do come back to this we need to find Josh' work to pass DSL code through to C++ run-time error messages such as dimension checking so that users can see the line of their code that resulted in the error. Perry thinks Josh' code might be hidden behind an option as it's not being used now based on the error messages we currently see.

@perrydv

perrydv commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

@paciorek We've deferred effort on your messaging work, but I could use it now so I am going to work on this. One thing I want to look at is using Rcpp and/or R API calls where possible, and I think in some cases that could replace the steps of looking up a function from the R environment and calling it.

@paciorek

Copy link
Copy Markdown
Contributor Author

Sounds good. I put a fair amount of thought into the user/developer experience here, so please keep me in the loop in terms of changes, particularly anything user-facing.

@perrydv

perrydv commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Hi @paciorek, I like a lot of this but have some questions and some changes I've drafted (just locally so far) but will run by you first:

  • We can consolidate several of the handlers in the generateCpp stage. This is purely internal.
  • For nStop, we can have the ultimate C++ call be Rcpp::stop. I think this is safer and cleaner in some way of recovering out of the call stack and possibly cleaning up objects better than when throwing a C++ error. Drafted.
  • For nWarning, similarly we can go through Rcpp::warning. Drafted.
  • For both of these and the others, I think it's better to make local std::ostringstream objects rather than one global one (which was nimble's approach). It will be simpler for object cleanup and also allow thread-safe behavior if multiple threads are giving errors or warnings. Drafted.
  • For nCat, we can use the Rcpp std output stream Rcpp:Rcout. Drafted.
  • nMessage is trickier because you've brought in the logging features using package logger. That looks useful, but I am wondering if we should keep it a distinct concept from "message", which is a standard R concept? One idea would be to have nMessage simply report a message and then introduce a new keyword like log_message or something else that uses the logging system. Easy to draft. What do you think?
  • I'm a little concerned about having the logger capitalized log levels become global names in both R and C++ that take over some common words. At least in C++ we could arrange otherwise. I guess in R we could rely on namespace or just accept that users of the logger package get these words reserved. What do you think if I modify this in R and/or C++?
  • If we go with the Rcpp tools as I'm suggesting, we can make a note later to look at package RcppThreads for some thread-safe versions. Easy to draft.
  • I think it would be nice to have nPrint (alias print). In nimble that is pretty much the same as cat, but we could make it a little more consistent with R's print, or decide it doesn't matter much. But I'd like to have it because cat is a bit nerdy and print is very natural. What do you think?
  • The progress bar feature is very nice. It is not ideal to look up an R function every time it is updated. If we create the bar by instantiating a C++ object (the RAII approach), we could do that look-up once and retain it. (But the down-side would be that we would need progress_update() to be called locally, in the same function where the progress bar was created, not in some other function. Or we would need to pass the object around.) I guess I'm just spitballing here and there is no need to change it unless we hear of performance issues.
  • Other tweaks and extensions occurred to me, but I'm not looking to put more effort in on this and the above seem relevant and simple for now.

Let me know what you think and I could make a PR to your branch.

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