Bioconductor / BiocParallel

Bioconductor facilities for parallel evaluation
https://bioconductor.org/packages/BiocParallel
65 stars 29 forks source link

update C++ code using cpp11 #202

Closed Jiefei-Wang closed 2 years ago

Jiefei-Wang commented 2 years ago

The argument and return type of the C++ functions have been changed to cpp11 type.

PeteHaitch commented 2 years ago

What's the advantage of this over the existing code?

Jiefei-Wang commented 2 years ago

Hi @PeteHaitch , here are two reasons for the change

  1. (main reason) the current implementation does not handle the C++ level exception properly. The boost library can throw exceptions in some special cases and, without the trycatch block, will crash the R, so we need to handle it properly.
  2. Since I'm working on this part, I think it will make future development easier if we rewrite the old code using the feature provided by cpp11. Therefore, we do not have to create the trycatch block ourself and the parameters for C++ functions are not an obscure type of SEXP, but a more specific C++ type.

I hope this can convince you of the changes.

Jiefei

PeteHaitch commented 2 years ago

Thanks, @Jiefei-Wang! Sorry, my question came across as a bit hostile, I apologise. I was just curious to understand the motivation.

Jiefei-Wang commented 2 years ago

No worries @PeteHaitch, it's my fault. This pull request is being reviewed by Martin so I incorrectly assume everyone knows the context. It is better to make it more clear:)

mtmorgan commented 2 years ago

The example on ?ipcreset fails

> ipcreset(id, 10)
Error: Invalid input type, expected 'integer' actual 'double'

because the updated code assumes the argument is an integer, whereas the original code coerced to integer.

-        {".ipc_lock", (DL_FUNC) & ipc_lock, 1},
+int ipc_n(cpp11::integers n_sexp)
 {
-    PROTECT(n_sexp = Rf_coerceVector(n_sexp, INTSXP));
-    bool test = IS_SCALAR(n_sexp, INTSXP) && (R_NaInt != Rf_asInteger(n_sexp));
-    if (!test)
-        Rf_error("'n' cannot be coerced to integer(1) and not NA");
-    int n = INTEGER(n_sexp)[0];
-    UNPROTECT(1);
-    return n;
+    if (n_sexp.size() != 1 || cpp11::is_na(n_sexp[0]))
+        Rf_error("'n' must be integer(1) and not NA");
+    return n_sexp[0];
 }

are there other similar problems?

Jiefei-Wang commented 2 years ago

hmm... I noticed this issue when I wrote the code, but I thought there will be an auto type conversion. I need to investigate it more...

Jiefei-Wang commented 2 years ago

Problem fixed. The integer value n will convert any numeric value to an integer. I added a new unit test in test_ipcmutex.R to avoid a similar issue happening.