search

eddelbuettel / rcppsimdjson

Rcpp Bindings for the 'simdjson' Header Library
116 stars 13 forks source link

Accomodate R use upstream in simdjson ? #1

Closed eddelbuettel closed 4 years ago

eddelbuettel commented 4 years ago

In 1449369ccc450617e5461d5d3d63be45bca22945 we replaced

because Writing R Extensions mandates it in Section 1.6.

It is a quick little edit and can be done again, but it is a recurrent problem. I.e. with RcppArmadillo and Armadillo we use a #define to switch from std::cout to Rcpp::Rcout (and could do likewiese for the error stream), in other places I use a typedef to switch from printf() to Rprintf(). But I have yet to find a really clever trick to switch from fprintf(stderr, ...) to REprintf(...).

Any ideas @lemire ?

Barring clever ideas, could this need be accomodates in the code generator in simdjson? Or too much complexity for too little gain?

lemire commented 4 years ago

We also have the syntax p("fdfdsdf") for errors introduced recently.

I think we can take this upstream (to simdjson) and see what can be done. The most useful contribution at this point would be to see how other upstream projects handle it. That is, I would rather not reinvent the wheel.

eddelbuettel commented 4 years ago

I will have a look at p("foo"). Sounds like something easy to overload/redefine on my side.

I am not aware of any great solutions---besides what Conrad and I came up with which works but may not even be called great (as I hate upper-case macros on stylistic grounds: looks like shouting).

In RcppArmadillo we have two defines overriding a defaukt upstream Armadillo has: https://github.com/RcppCore/RcppArmadillo/blob/cf59a3c5b928425d99d4b1378c6e40a99913c1f0/inst/include/RcppArmadilloConfig.h#L76-L87
Throughout the Armadillo codebase, Conrad diligently uses ARMA_COUT_STREAM and ARMA_CERR_STREAM.

And years later I still wonder if there could be a better way. I have not found one.

lemire commented 4 years ago

Ok. The solution is to remove all instances from the main library. I don't think we have many.

eddelbuettel commented 4 years ago

Yes, see this commit for a quick and dirty direct hack (which I'd of course rather avoid repeatedly): https://github.com/eddelbuettel/rcppsimdjson/commit/1449369ccc450617e5461d5d3d63be45bca22945

After that, R CMD check was happy.

lemire commented 4 years ago

See https://github.com/lemire/simdjson/pull/455

lemire commented 4 years ago

Hopefully, I caught them all.

lemire commented 4 years ago

I have to let it run all the tests, and then I shall merge. I think that this issue was raised before.

Of course, we do not want users of the library to have to hack it.

eddelbuettel commented 4 years ago

Yes, I realized over the quick lunch break in the kitchen that my thinking was sloppy as I only considered the (generated) subset in the single-header directory. Thanks for making the change -- I only glanced at it now.

lemire commented 4 years ago

I don't see how your thinking was sloppy.

In simdjson, we have separate files and only produce an amalgamated version occasionally. You would not know this ordinarily.

eddelbuettel commented 4 years ago

One can of course follow commit logs, so many thanks for making the change upstream! I will keep an on the singleheader/ directory and catch up once it changes.

I may keep this open til then though I of course gratefullu acknowledge that you did make the change already---much appreciated. "Downstream" use and maintainership is always best (and easiest) with such help and support from "upstream".