5
\$\begingroup\$

I want to implement a cumulative all through R's C interface. I was able to hack something together that produces the correct results, but I have little idea what idiomatic C should look like. The code should be easy to understand, even if you don't know about R.

Description

The cumulative all takes a LGLSXP (logical S-expression) input. The function should output a LGLSXP of the same length as the input, where each element represents whether all values up to this point were true.

The three valued logic works like this: (true & false) -> false, (true & na) -> na, (false & na) -> false

LGLSXP

A LGLSXP is a pointer to a struct that contains an int array that represents 3 valued logic with 0 as false, INT_MIN as NA and 1 as true. This array can be accessed with LOGICAL(). You can read about SEXPs here.

Code

#include <R.h>
#include <Rinternals.h>

SEXP cumall_impl(SEXP x) {
  SEXP res = PROTECT(allocVector(LGLSXP, XLENGTH(x)));
  R_xlen_t n = XLENGTH(x);

  if (n == 0){
    UNPROTECT(1);
    return(res);
  } else {
    memset(LOGICAL(res), 0, n * sizeof(int));
  }

  int *p_x = LOGICAL(x);
  int *p_res = LOGICAL(res);
  p_res[0] = p_x[0];

  for (R_xlen_t i = 1; i < n; i++) {
    if (p_x[i] == TRUE)
      p_res[i] = p_res[i - 1];
    else if (p_x[i] == NA_LOGICAL)
      p_res[i] = NA_LOGICAL;
    else
      break;
  }

  UNPROTECT(1);
  return res;
}

Remarks

  • PROTECT() and UNPROTECT() are necessary to prevent R's garbage collector from interfering
  • LOGICAL() accesses the underlying array of a LGLSXP
  • TRUE, NA_LOGICAL etc are provided by the headers.

Headers

I am thankful for all suggestions, be it coding style, variable naming or critique to how I formulated my question

\$\endgroup\$
4
  • \$\begingroup\$ "everything else as true" and if (p_x[i] == TRUE) seems wrong. I'd expect all values that are neither NA_LOGICAL nor FALSE, not just the one TRUE, to cause that if() test to be true. If we can rely on 2's complement, perhaps if (p_x[i] & INT_MAX) \$\endgroup\$ Commented Aug 15, 2018 at 1:29
  • \$\begingroup\$ When you say "LGLSXP is implemented as an integer array...." do you mean "... int array...". If so better to say int. If not always an int, sizeof(int) in memset(LOGICAL(res), 0, n * sizeof(int)); is suspicious. I guess it comes down to it would make for a more effective review with <R.h>, <Rinternals.h> posted or referenced. \$\endgroup\$ Commented Aug 15, 2018 at 1:36
  • \$\begingroup\$ Thanks. I added a link to Rinternals.h and R.h to the original question. I have since rewritten the code using the suggestions by Edward, but I am not sure if I should post the new code here or not. LGLSXP should only take the values TRUE, FALSE, NA_LOGICAL. If there are different values something is not right and the function should imho abort. \$\endgroup\$
    – hoelk
    Commented Aug 15, 2018 at 7:10
  • \$\begingroup\$ I figured out I am not 100% sure how TRUE is defined (but in C everything that does not evaluate to 0 evaluates to true, right?). I added some more links to headers that seemed relevant to the question. \$\endgroup\$
    – hoelk
    Commented Aug 15, 2018 at 7:31

1 Answer 1

3
\$\begingroup\$

I see some things that may help you improve your code.

Use variables effectively

The code begins with this:

SEXP cumall_impl(SEXP x) {
    SEXP res = PROTECT(allocVector(LGLSXP, XLENGTH(x)));
    R_xlen_t n = XLENGTH(x);

However, there's not really any reason not to define and use n first:

SEXP cumall_impl(SEXP x) {
    R_xlen_t n = XLENGTH(x);
    SEXP res = PROTECT(allocVector(LGLSXP, n));

Use all required #includes

The code uses memset and so it should also have this line:

#include <string.h>

Even though it happens to compile because some of the R libraries probably also include that, it's important to make sure that the code does not rely on undocumented interface assumptions.

Use a single loop over all elements

The code uses memset to initialize all values to FALSE and then sets the first value explicitly and then loops through all other values. I'd be inclined instead to recode that as a single for loop instead:

int prev = TRUE;
for (R_xlen_t i = 0; i < n; ++i) {
    if (p_x[i] == TRUE)
        p_res[i] = prev;
    else if (p_x[i] == NA_LOGICAL)
        p_res[i] = NA_LOGICAL;
    else
        p_res[i] = FALSE;
    prev = p_res[i];
}

Writing it this way avoids the need for the if (n == 0) construct and also eliminates the need for the memset.

Note: Ordinarily, we'd prefer to use a switch there instead of a chained if..else but we're prevented from doing so because NA_LOGICAL does not evaluate to an integer constant the way it's defined in R.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the tips. Some of those I should have caught myself (like the n). \$\endgroup\$
    – hoelk
    Commented Aug 14, 2018 at 5:30
  • \$\begingroup\$ Do you maybe have any more tips where I can look up where stuff like TRUE, NA_LOGICAL, etc are defined? is there a better way than just googling? \$\endgroup\$
    – hoelk
    Commented Aug 14, 2018 at 5:42
  • 1
    \$\begingroup\$ You can look up the definitions by reading through the #include files R.h and Rinternals.h but be aware that unless it's a documented part of the interface, it may well change. Documentation is here: cran.r-project.org/doc/manuals/fullrefman.pdf \$\endgroup\$
    – Edward
    Commented Aug 14, 2018 at 13:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.