Skip to content

chore: Replace Rcpp by cpp11 - #441

Merged
krlmlr merged 43 commits into
r-dbi:mainfrom
Antonov548:cpp11
Feb 13, 2023
Merged

chore: Replace Rcpp by cpp11 #441
krlmlr merged 43 commits into
r-dbi:mainfrom
Antonov548:cpp11

Conversation

@Antonov548

@Antonov548 Antonov548 commented Oct 26, 2022

Copy link
Copy Markdown
Contributor
  • busy_callback_helper() -> how to catch eval error and interrupt?
  • result_release() -> make this work with cpp11::external_pointer
  • find replacement for Rcpp::warningcall(R_NilValue, std::string("Ignoring remaining part of query: ") + tail);
  • figure out why cpp11::warning() crashes
  • adapt astyle()
  • maybe remove some STRICT_R_HEADERS and R_NO_REMAP

@Antonov548

Copy link
Copy Markdown
Contributor Author

@krlmlr
Seems cpp11 hasn't any functions to handle exceptions from R functions called in C++ via cpp11::function.
In Rcpp they adding try catch block around functions passed from R to C++ also by installing additional scripts.

We can implement this logic in RSQLite just using R native functions or just remove catching of errors from R, at least for now.

@krlmlr

krlmlr commented Oct 27, 2022

Copy link
Copy Markdown
Member

I remember seeing a safe modifier, e.g., in https://github.com/cran/duckdb/blob/f2550069da4019ebb5f81103dd92d471c40d4d3b/inst/include/cpp11/list.hpp#L92

Would that help?

@krlmlr

krlmlr commented Oct 27, 2022

Copy link
Copy Markdown
Member

I also wonder why you commit with a different committer name and e-mail. Is this from gitpod?

@Antonov548

Copy link
Copy Markdown
Contributor Author

Yes, this is due to the gitpod, I also wondering why it sometime from your name.
Maybe it's due gitconfig changes in gitpod configuration in RMariaDB project - https://github.com/r-dbi/RMariaDB/blob/66f890fdf08489b76e1a3dea20db28c1a15c9aba/.gitpod.yml#L7

@Antonov548

Copy link
Copy Markdown
Contributor Author

I remember seeing a safe modifier, e.g., in https://github.com/cran/duckdb/blob/f2550069da4019ebb5f81103dd92d471c40d4d3b/inst/include/cpp11/list.hpp#L92

Would that help?

Yes, I didn't think about safe object. I will take a look, thanks.

@krlmlr

krlmlr commented Oct 27, 2022

Copy link
Copy Markdown
Member

Thanks, weird. Can you please try this in a clean repository that doesn't have a .gitpod.yml ?

@Antonov548

Copy link
Copy Markdown
Contributor Author

Thanks, weird. Can you please try this in a clean repository that doesn't have a .gitpod.yml ?

Yes, I will try

@krlmlr krlmlr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we vendor the cpp11 package with cpp11::vendor_cpp11() and fix the tests that are now skipped?

Comment thread src/rsqlite.cpp
@krlmlr

krlmlr commented Feb 1, 2023

Copy link
Copy Markdown
Member

We should un-skip the tests and fix the code instead. @Antonov548: can you please take a look?

@krlmlr

krlmlr commented Feb 5, 2023

Copy link
Copy Markdown
Member

@Antonov548: The "interrupt" test still fails legitimately. To reproduce, the package needs to be installed with R CMD INSTALL . from the same branch, we've omitted this today. Can you please take a look?

@Antonov548 Antonov548 changed the title Replace Rcpp by cpp11 chore: Replace Rcpp by cpp11 Feb 8, 2023
@krlmlr
krlmlr merged commit 045ba88 into r-dbi:main Feb 13, 2023
@krlmlr
krlmlr deleted the cpp11 branch February 13, 2023 07:50
@krlmlr

krlmlr commented Feb 13, 2023

Copy link
Copy Markdown
Member

Thanks!

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants