Plain C implementation of 'Cumulative All' operation for R

Question

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

• The most important definitions are in Rinternals.h and R.h.
• The NA_LOGICAL is defined in Arith.h and arithmetic.c
• I am not sure where/how TRUE and FALSE are really defined. Likely sources are Boolean.h and Rdefines.h

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

Answer 1

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.