various constness, control flow, and object placement improvements in freadR

Author: badasahog

src/freadR.c

@@ -77,7 +77,8 @@ SEXP freadR(
   SEXP encodingArg,
   SEXP keepLeadingZerosArgs,
   SEXP noTZasUTC
-) {
+)
+{
   verbose = LOGICAL(verboseArg)[0];
   warningsAreErrors = LOGICAL(warnings2errorsArg)[0];
 
@@ -227,14 +228,16 @@ SEXP freadR(
   return DT;
 }
 
-static void applyDrop(SEXP items, int8_t *type, int ncol, int dropSource) {
+static void applyDrop(SEXP items, int8_t *type, int ncol, int dropSource)
+{
   if (!length(items)) return;
-  SEXP itemsInt = PROTECT(isString(items) ? chmatch(items, colNamesSxp, NA_INTEGER) : coerceVector(items, INTSXP));
-  const int *itemsD = INTEGER(itemsInt), n=LENGTH(itemsInt);
+  const SEXP itemsInt = PROTECT(isString(items) ? chmatch(items, colNamesSxp, NA_INTEGER) : coerceVector(items, INTSXP));
+  const int* const itemsD = INTEGER(itemsInt);
+  const int n = LENGTH(itemsInt);
   for (int j = 0; j < n; j++) {
-    int k = itemsD[j];
+    const int k = itemsD[j];
     if (k == NA_INTEGER || k < 1 || k > ncol) {
-      static char buff[50];
+      char buff[50];
       if (dropSource == -1) snprintf(buff, sizeof(buff), "drop[%d]", j + 1); // # notranslate
       else snprintf(buff, sizeof(buff), "colClasses[[%d]][%d]", dropSource + 1, j + 1); // # notranslate
       if (k == NA_INTEGER) {
@@ -258,7 +261,7 @@ bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, const int
 {
   // use typeSize superfluously to avoid not-used warning; otherwise could move typeSize from fread.h into fread.c
   if (typeSize[CT_BOOL8_N] != 1) internal_error(__func__, "typeSize[CT_BOOL8_N] != 1"); // # nocov
-  if (typeSize[CT_STRING] != 8) internal_error(__func__, "typeSize[CT_STRING] != 1"); // # nocov
+  if (typeSize[CT_STRING] != 8) internal_error(__func__, "typeSize[CT_STRING] != 8"); // # nocov
   colNamesSxp = R_NilValue;
   SET_VECTOR_ELT(RCHK, 1, colNamesSxp = allocVector(STRSXP, ncol));
   for (int i = 0; i < ncol; i++) {
@@ -300,7 +303,7 @@ bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, const int
     SET_VECTOR_ELT(RCHK, 3, selectRank = allocVector(INTSXP, ncol));
     int *selectRankD = INTEGER(selectRank), rank = 1;
     for (int i = 0; i < n; i++) {
-      int k = selectInts[i];
+      const int k = selectInts[i];
       if (k == NA_INTEGER) continue; // missing column name warned above and skipped
       if (k < 0) STOP(_("Column number %d (select[%d]) is negative but should be in the range [1,ncol=%d]. Consider drop= for column exclusion."), k, i + 1, ncol);
       if (k == 0) STOP(_("select = 0 (select[%d]) has no meaning. All values of select should be in the range [1,ncol=%d]."), i + 1, ncol);
@@ -335,7 +338,7 @@ bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, const int
           if (type[i] == CT_DROP) continue;                    // user might have specified the type of all columns including those dropped with drop=
           const SEXP tt = STRING_ELT(colClassesSxp, i & mask); // mask recycles colClassesSxp when it's length-1
           if (tt == NA_STRING || tt == R_BlankString) continue;  // user is ok with inherent type for this column
-          int w = INTEGER(typeEnum_idx)[i & mask];
+          const int w = INTEGER(typeEnum_idx)[i & mask];
           if (tt == char_POSIXct) {
             // from v1.13.0, POSIXct is a built in type, but if the built-in doesn't support (e.g. test 1743.25 has missing tzone) then we still dispatch to as.POSIXct afterwards
             if (type[i] != CT_ISO8601_TIME) {
@@ -354,8 +357,8 @@ bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, const int
         for (int i = 0; i < n; i++) {
           SEXP tt = STRING_ELT(colClassesSxp, i);
           if (tt == NA_STRING || tt == R_BlankString) continue;
-          int w = INTEGER(typeEnum_idx)[i];
-          int y = selectInts[i];
+          const int w = INTEGER(typeEnum_idx)[i];
+          const int y = selectInts[i];
           if (y == NA_INTEGER) continue;
           if (tt == char_POSIXct) {
             if (type[y - 1] != CT_ISO8601_TIME) {
@@ -391,7 +394,7 @@ bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, const int
         else                 itemsInt = PROTECT(coerceVector(items, INTSXP));
         // UNPROTECTed directly just after this for loop. No nprotect++ here is correct.
         for (int j = 0; j < LENGTH(items); j++) {
-          int colIdx = INTEGER(itemsInt)[j]; // NB: 1-based
+          const int colIdx = INTEGER(itemsInt)[j]; // NB: 1-based
           if (colIdx == NA_INTEGER) {
             if (isString(items))
               DTWARN(_("Column name '%s' (colClasses[[%d]][%d]) not found"), CHAR(STRING_ELT(items, j)), i + 1, j + 1);
@@ -432,18 +435,19 @@ bool userOverride(int8_t *type, lenOff *colNames, const char *anchor, const int
     }
     UNPROTECT(1);  // typeRName_sxp
   }
-  UNPROTECT(nprotect);
+  if(nprotect) UNPROTECT(nprotect);
   if (readInt64As != CT_INT64) {
     for (int i = 0; i < ncol; i++) if (type[i] == CT_INT64) type[i] = readInt64As;
   }
   return true;
 }
 
-size_t allocateDT(int8_t *typeArg, int8_t *sizeArg, int ncolArg, int ndrop, size_t allocNrow) {
+size_t allocateDT(int8_t *typeArg, int8_t *sizeArg, int ncolArg, int ndrop, size_t allocNrow)
+{
   // save inputs for use by pushBuffer
   size = sizeArg;
   type = typeArg;
-  int newDT = (ncol == 0);
+  const bool newDT = (ncol == 0);
   if (newDT) {
     ncol = ncolArg;
     dtnrows = allocNrow;
@@ -453,20 +457,28 @@ size_t allocateDT(int8_t *typeArg, int8_t *sizeArg, int ncolArg, int ndrop, size
       if (colClassesAs) setAttrib(DT, sym_colClassesAs, colClassesAs);
     } else {
       int nprotect = 0;
-      SEXP tt, ss = R_NilValue;
-      setAttrib(DT, R_NamesSymbol, tt = PROTECT(allocVector(STRSXP, ncol - ndrop))); nprotect++;
+      SEXP tt = PROTECT(allocVector(STRSXP, ncol - ndrop));
+      setAttrib(DT, R_NamesSymbol, tt); nprotect++;
+      
+      SEXP ss;
       if (colClassesAs) {
-        setAttrib(DT, sym_colClassesAs, ss = PROTECT(allocVector(STRSXP, ncol - ndrop))); nprotect++;
+        ss = PROTECT(allocVector(STRSXP, ncol - ndrop)); nprotect++;
+        setAttrib(DT, sym_colClassesAs, ss);
+      } else {
+        ss = R_NilValue;
       }
+
       for (int i = 0, resi = 0; i < ncol; i++) if (type[i] != CT_DROP) {
         if (colClassesAs) SET_STRING_ELT(ss, resi, STRING_ELT(colClassesAs, i));
         SET_STRING_ELT(tt, resi++, STRING_ELT(colNamesSxp, i));
       }
+
       UNPROTECT(nprotect);
     }
     if (selectRank) {
       SEXP tt = PROTECT(allocVector(INTSXP, ncol - ndrop));
-      int *ttD = INTEGER(tt), *rankD = INTEGER(selectRank), rank = 1;
+      int *ttD = INTEGER(tt), rank = 1;
+      const int *rankD = INTEGER(selectRank);
       for (int i = 0; i < ncol; i++) if (type[i] != CT_DROP) ttD[rankD[i] - 1] = rank++;
       SET_VECTOR_ELT(RCHK, 3, selectRank = tt);
       // selectRank now holds the order not the rank (so its name is now misleading). setFinalNRow passes it to setcolorder
@@ -494,13 +506,13 @@ size_t allocateDT(int8_t *typeArg, int8_t *sizeArg, int ncolArg, int ndrop, size
   for (int i = 0, resi = 0; i < ncol; i++) {
     if (type[i] == CT_DROP) continue;
     SEXP col = VECTOR_ELT(DT, resi);
-    int oldIsInt64 = newDT ? 0 : INHERITS(col, char_integer64);
-    int newIsInt64 = type[i] == CT_INT64;
-    int typeChanged = (type[i] > 0) && (newDT || TYPEOF(col) != typeSxp[type[i]] || oldIsInt64 != newIsInt64);
-    int nrowChanged = (allocNrow != dtnrows);
+    const bool oldIsInt64 = newDT ? 0 : INHERITS(col, char_integer64);
+    const bool newIsInt64 = type[i] == CT_INT64;
+    const bool typeChanged = (type[i] > 0) && (newDT || TYPEOF(col) != typeSxp[type[i]] || oldIsInt64 != newIsInt64);
+    const bool nrowChanged = (allocNrow != dtnrows);
     if (typeChanged || nrowChanged) {
-      SEXP thiscol = typeChanged ? allocVector(typeSxp[type[i]], allocNrow)  // no need to PROTECT, passed immediately to SET_VECTOR_ELT, see R-exts 5.9.1
-                                 : growVector(col, allocNrow);
+      SEXP thiscol = typeChanged ? allocVector(typeSxp[type[i]], allocNrow) : growVector(col, allocNrow); // no need to PROTECT, passed immediately to SET_VECTOR_ELT, see R-exts 5.9.1
+      
       SET_VECTOR_ELT(DT, resi, thiscol);
       if (type[i] == CT_INT64) {
         SEXP tt = PROTECT(ScalarString(char_integer64));
@@ -530,7 +542,8 @@ size_t allocateDT(int8_t *typeArg, int8_t *sizeArg, int ncolArg, int ndrop, size
   return DTbytes;
 }
 
-void setFinalNrow(size_t nrow) {
+void setFinalNrow(size_t nrow)
+{
   if (selectRank) setcolorder(DT, selectRank);  // selectRank was changed to contain order (not rank) in allocateDT above
   if (length(DT)) {
     if (nrow == dtnrows)
@@ -545,9 +558,10 @@ void setFinalNrow(size_t nrow) {
   R_FlushConsole(); // # 2481. Just a convenient place; nothing per se to do with setFinalNrow()
 }
 
-void dropFilledCols(int* dropArg, int ndelete) {
+void dropFilledCols(int* dropArg, int ndelete)
+{
   dropFill = dropArg;
-  int ndt = length(DT);
+  const int ndt = length(DT);
   for (int i = 0; i < ndelete; i++) {
     SET_VECTOR_ELT(DT, dropFill[i], R_NilValue);
     SET_STRING_ELT(colNamesSxp, dropFill[i], NA_STRING);
@@ -562,13 +576,13 @@ void pushBuffer(ThreadLocalFreadParsingContext *ctx)
   const void *buff4 = ctx->buff4;
   const void *buff1 = ctx->buff1;
   const char *anchor = ctx->anchor;
-  int nRows = (int) ctx->nRows;
-  size_t DTi = ctx->DTi;
-  int rowSize8 = (int) ctx->rowSize8;
-  int rowSize4 = (int) ctx->rowSize4;
-  int rowSize1 = (int) ctx->rowSize1;
-  int nStringCols = ctx->nStringCols;
-  int nNonStringCols = ctx->nNonStringCols;
+  const size_t nRows = ctx->nRows;
+  const size_t DTi = ctx->DTi;
+  const size_t rowSize8 = ctx->rowSize8;
+  const size_t rowSize4 = ctx->rowSize4;
+  const size_t rowSize1 = ctx->rowSize1;
+  const int nStringCols = ctx->nStringCols;
+  const int nNonStringCols = ctx->nNonStringCols;
 
   // Do all the string columns first so as to minimize and concentrate the time inside the single critical.
   // While the string columns are happening other threads before me can be copying their non-string buffers to the
@@ -581,14 +595,14 @@ void pushBuffer(ThreadLocalFreadParsingContext *ctx)
     #pragma omp critical
     {
       int off8 = 0;
-      int cnt8 = rowSize8 / 8;
-      lenOff *buff8_lenoffs = (lenOff*) buff8;
+      const int cnt8 = rowSize8 / 8;
+      const lenOff *buff8_lenoffs = (const lenOff*)buff8;
       for (int j = 0, resj = -1, done = 0; done < nStringCols && j < ncol; j++) {
         if (type[j] == CT_DROP) continue;
         resj++;
         if (type[j] == CT_STRING) {
           SEXP dest = VECTOR_ELT(DT, resj);
-          lenOff *source = buff8_lenoffs + off8;
+          const lenOff *source = buff8_lenoffs + off8;
           for (int i = 0; i < nRows; i++) {
             int strLen = source->len;
             if (strLen <= 0) {
@@ -600,7 +614,7 @@ void pushBuffer(ThreadLocalFreadParsingContext *ctx)
               while (c < strLen && str[c]) c++;
               if (c < strLen) {
                 // embedded nul found; any at the beginning or the end of the field should have already been excluded but this will strip those too if present just in case
-                char *last = (char *)str + c;    // obtain write access to (const char *)anchor;
+                char *last = (char*)str + c;    // obtain write access to (const char *)anchor;
                 while (c < strLen) {
                   if (str[c]) *last++ = str[c];  // cow page write: saves allocation and management of a temp that would need to thread-safe in future.
                   c++;                         //   This is only thread accessing this region. For non-mmap direct input nul are not possible (R would not have accepted nul earlier).
@@ -613,55 +627,69 @@ void pushBuffer(ThreadLocalFreadParsingContext *ctx)
           }
           done++; // if just one string col near the start, don't loop over the other 10,000 cols. TODO? start on first too
         }
-        off8 += (size[j] == 8);
+
+        if (size[j] == 8) off8++;
       }
     }
   }
 
   int off1 = 0, off4 = 0, off8 = 0;
-  for (int j = 0, resj = -1, done = 0; done < nNonStringCols && j < ncol; j++) {
+  for (int j = 0, resj = 0, done = 0; done < nNonStringCols && j < ncol; j++) {
     if (type[j] == CT_DROP) continue;
-    int thisSize = size[j];
-    resj++;
+    
     if (type[j] != CT_STRING && type[j] > 0) {
-      if (thisSize == 8) {
+      switch(size[j])
+      {
+      case 8: {
         double *dest = REAL(VECTOR_ELT(DT, resj)) + DTi;
-        const char *src8 = (char*)buff8 + off8;
+        const char *src8 = (const char*)buff8 + off8;
         for (int i = 0; i < nRows; i++) {
-          *dest = *(double *)src8;
+          *dest = *(double*)src8;
           src8 += rowSize8;
           dest++;
         }
-      } else if (thisSize == 4) {
+        break;
+      }
+      case 4: {
         int *dest = INTEGER(VECTOR_ELT(DT, resj)) + DTi;
-        const char *src4 = (char*)buff4 + off4;
+        const char *src4 = (const char*)buff4 + off4;
         // debug line for #3369 ... if (DTi>2638000) printf("freadR.c:460: thisSize==4, resj=%d, %"PRIu64", %d, %d, j=%d, done=%d\n", resj, (uint64_t)DTi, off4, rowSize4, j, done);
         for (int i = 0; i < nRows; i++) {
-          *dest = *(int *)src4;
+          *dest = *(int*)src4;
           src4 += rowSize4;
           dest++;
         }
-      } else if (thisSize == 1) {
+        break;
+      }
+      case 1: {
         if (type[j] > CT_BOOL8_Y) STOP(_("Field size is 1 but the field is of type %d\n"), type[j]);
         int *dest = LOGICAL(VECTOR_ELT(DT, resj)) + DTi;
-        const char *src1 = (char*)buff1 + off1;
+        const char *src1 = (const char*)buff1 + off1;
         for (int i = 0; i < nRows; i++) {
-          int8_t v = *(int8_t *)src1;
+          const int8_t v = *(int8_t*)src1;
           *dest = (v == INT8_MIN ? NA_INTEGER : v);
           src1 += rowSize1;
           dest++;
         }
-      } else internal_error(__func__, "unexpected field of size %d\n", thisSize);  // # nocov
+        break;
+      }
+      default: { // # nocov
+        internal_error(__func__, "unexpected field of size %d\n", size[j]); // # nocov
+        break; // # nocov
+      }
+      }
       done++;
     }
     off8 += (size[j] & 8);
     off4 += (size[j] & 4);
     off1 += (size[j] & 1);
+    resj++;
   }
 }
 
 // # nocov start
-void progress(int p, int eta) {
+void progress(int p, int eta)
+{
   // called from thread 0 only
   // p between 0 and 100
   // eta in seconds
@@ -707,7 +735,8 @@ void progress(int p, int eta) {
 }
 // # nocov end
 
-void halt__(bool warn, const char *format, ...) {
+void halt__(bool warn, const char *format, ...)
+{
   // Solves: http://stackoverflow.com/questions/18597123/fread-data-table-locks-files
   // TODO: always include fnam in the STOP message. For log files etc.
   va_list args;