From 7f9ff44c60b778c1252ceeb484c565326e83033d Mon Sep 17 00:00:00 2001 From: Matt Dowle Date: Thu, 6 Oct 2022 16:05:04 -0600 Subject: [PATCH] pass -Wstrict-prototypes for CRAN (#5477) --- .dev/cc.R | 3 +- NEWS.md | 4 +- src/assign.c | 4 +- src/data.table.h | 83 ++++++++++++++++++++++++++++++++++--- src/forder.c | 10 ++--- src/fread.c | 2 +- src/fwrite.c | 53 ++++++++++++------------ src/fwrite.h | 35 ++++++++-------- src/fwriteR.c | 10 ++--- src/gsumm.c | 62 ++++++++++++++-------------- src/init.c | 101 ++------------------------------------------- src/openmp-utils.c | 8 ++-- src/snprintf.c | 2 +- src/utils.c | 2 +- 14 files changed, 181 insertions(+), 198 deletions(-) diff --git a/.dev/cc.R b/.dev/cc.R index bc15b6765..2d60e4200 100644 --- a/.dev/cc.R +++ b/.dev/cc.R @@ -61,7 +61,8 @@ cc = function(test=FALSE, clean=FALSE, debug=FALSE, omp=!debug, cc_dir, path=Sys if (debug) { ret = system(sprintf("MAKEFLAGS='-j CC=%s PKG_CFLAGS=-f%sopenmp CFLAGS=-std=c99\\ -O0\\ -ggdb\\ -pedantic' R CMD SHLIB -d -o data_table.so *.c", CC, OMP)) } else { - ret = system(sprintf("MAKEFLAGS='-j CC=%s CFLAGS=-f%sopenmp\\ -std=c99\\ -O3\\ -pipe\\ -Wall\\ -pedantic\\ -fno-common' R CMD SHLIB -o data_table.so *.c", CC, OMP)) + ret = system(sprintf("MAKEFLAGS='-j CC=%s CFLAGS=-f%sopenmp\\ -std=c99\\ -O3\\ -pipe\\ -Wall\\ -pedantic\\ -Wstrict-prototypes\\ -isystem\\ /usr/share/R/include\\ -fno-common' R CMD SHLIB -o data_table.so *.c", CC, OMP)) + # the -isystem suppresses strict-prototypes warnings from R's headers. Look at the output to see what -I is and pass the same path to -isystem. # TODO add -Wextra too? } if (ret) return() diff --git a/NEWS.md b/NEWS.md index f63e20673..ee808e2ac 100644 --- a/NEWS.md +++ b/NEWS.md @@ -610,10 +610,12 @@ ## NOTES -1. gcc 12.1 (May 2022) now detects and warns about an always-false condition in `fread` which caused a small efficiency saving never to be invoked. Thanks to CRAN for testing latest versions of compilers. +1. gcc 12.1 (May 2022) now detects and warns about an always-false condition (`-Waddress`) in `fread` which caused a small efficiency saving never to be invoked. Thanks to CRAN for testing latest versions of compilers. 2. `update.dev.pkg()` has been renamed `update_dev_pkg()` to get out of the way of the `stats::update` generic function, [#5421](https://github.com/Rdatatable/data.table/pull/5421). This is a utility function which upgrades the version of `data.table` to the latest commit in development which has passed all tests. As such we don't expect any backwards compatibility concerns. Its manual page was causing an intermittent hang/crash from `R CMD check` on Windows-only on CRAN which we hope will be worked around by changing its name. +3. Internal C code now passes `-Wstrict-prototypes` to satisfy the warnings now displayed on CRAN. + # data.table [v1.14.2](https://github.com/Rdatatable/data.table/milestone/24?closed=1) (27 Sep 2021) diff --git a/src/assign.c b/src/assign.c index 7fb09fa71..f48f71e73 100644 --- a/src/assign.c +++ b/src/assign.c @@ -1190,7 +1190,7 @@ SEXP allocNAVectorLike(SEXP x, R_len_t n) { static SEXP *saveds=NULL; static R_len_t *savedtl=NULL, nalloc=0, nsaved=0; -void savetl_init() { +void savetl_init(void) { if (nsaved || nalloc || saveds || savedtl) { error(_("Internal error: savetl_init checks failed (%d %d %p %p). please report to data.table issue tracker."), nsaved, nalloc, saveds, savedtl); // # nocov } @@ -1231,7 +1231,7 @@ void savetl(SEXP s) nsaved++; } -void savetl_end() { +void savetl_end(void) { // Can get called if nothing has been saved yet (nsaved==0), or even if _init() hasn't been called yet (pointers NULL). Such // as to clear up before error. Also, it might be that nothing needed to be saved anyway. for (int i=0; i> approach to cleanup() on error. */ -static void free_ustr() { +static void free_ustr(void) { for(int i=0; i + #include // for SEXP in writeList() prototype #include "po.h" #define STOP error #define DTPRINT Rprintf #endif -typedef void (*writer_fun_t)(const void *, int64_t, char **); +typedef void writer_fun_t(const void *, int64_t, char **); // in the order of writer_fun_t in fwriteR.c -void writeBool8(); -void writeBool32(); -void writeBool32AsString(); -void writeInt32(); -void writeInt64(); -void writeFloat64(); -void writeComplex(); -void writeITime(); -void writeDateInt32(); -void writeDateFloat64(); -void writePOSIXct(); -void writeNanotime(); -void writeString(); -void writeCategString(); -void writeList(); +writer_fun_t writeBool8; +writer_fun_t writeBool32; +writer_fun_t writeBool32AsString; +writer_fun_t writeInt32; +writer_fun_t writeInt64; +writer_fun_t writeFloat64; +writer_fun_t writeComplex; +writer_fun_t writeITime; +writer_fun_t writeDateInt32; +writer_fun_t writeDateFloat64; +writer_fun_t writePOSIXct; +writer_fun_t writeNanotime; +writer_fun_t writeString; +writer_fun_t writeCategString; +writer_fun_t writeList; void write_chars(const char *source, char **dest); @@ -75,7 +76,7 @@ typedef struct fwriteMainArgs int64_t nrow; // a vector of pointers to all-same-length column vectors const void **columns; - writer_fun_t *funs; // a vector of writer_fun_t function pointers + writer_fun_t **funs; // a vector of writer_fun_t function pointers // length ncol vector containing which fun[] to use for each column // one byte to use 8 times less cache lines than a vector of function pointers would do diff --git a/src/fwriteR.c b/src/fwriteR.c index a36e44315..f64768d70 100644 --- a/src/fwriteR.c +++ b/src/fwriteR.c @@ -19,7 +19,7 @@ static const char *sep2start, *sep2end; // if there are no list columns, set sep2=='\0' // Non-agnostic helpers ... -const char *getString(SEXP *col, int64_t row) { // TODO: inline for use in fwrite.c +const char *getString(const SEXP *col, int64_t row) { // TODO: inline for use in fwrite.c SEXP x = col[row]; return x==NA_STRING ? NULL : ENCODED_CHAR(x); } @@ -53,7 +53,7 @@ const char *getCategString(SEXP col, int64_t row) { return x==NA_INTEGER ? NULL : ENCODED_CHAR(STRING_ELT(getAttrib(col, R_LevelsSymbol), x-1)); } -writer_fun_t funs[] = { +writer_fun_t *funs[] = { &writeBool8, &writeBool32, &writeBool32AsString, @@ -73,8 +73,8 @@ writer_fun_t funs[] = { static int32_t whichWriter(SEXP); -void writeList(SEXP *col, int64_t row, char **pch) { - SEXP v = col[row]; +void writeList(const void *col, int64_t row, char **pch) { + SEXP v = ((const SEXP *)col)[row]; int32_t wf = whichWriter(v); if (TYPEOF(v)==VECSXP || wf==INT32_MIN || isFactor(v)) { error(_("Internal error: getMaxListItemLen should have caught this up front.")); // # nocov @@ -82,7 +82,7 @@ void writeList(SEXP *col, int64_t row, char **pch) { char *ch = *pch; write_chars(sep2start, &ch); const void *data = DATAPTR_RO(v); - writer_fun_t fun = funs[wf]; + writer_fun_t *fun = funs[wf]; for (int j=0; j16) shift=nb/2; // TODO: when we have stress-test off mode, do this - mask = (1<>shift) + 1; + bitshift = nb/2; // /2 so that high and low can be uint16_t, and no limit (even for nb=4) to stress-test. + // bitshift=MAX(nb-8,0); if (bitshift>16) bitshift=nb/2; // TODO: when we have stress-test off mode, do this + mask = (1<>bitshift) + 1; grp = (int *)R_alloc(nrow, sizeof(int)); // TODO: use malloc and made this local as not needed globally when all functions here use gather // maybe better to malloc to avoid R's heap. This grp isn't global, so it doesn't need to be R_alloc @@ -86,8 +86,8 @@ SEXP gforce(SEXP env, SEXP jsub, SEXP o, SEXP f, SEXP l, SEXP irowsArg) { // TODO: enable stress-test mode in tests only (#3205) which can be turned off by default in release to decrease overhead on small data // if that is established to be biting (it may be fine). if (nBatch<1 || batchSize<1 || lastBatchSize<1) { - error(_("Internal error: nrow=%d ngrp=%d nbit=%d shift=%d highSize=%d nBatch=%d batchSize=%d lastBatchSize=%d\n"), // # nocov - nrow, ngrp, nb, shift, highSize, nBatch, batchSize, lastBatchSize); // # nocov + error(_("Internal error: nrow=%d ngrp=%d nbit=%d bitshift=%d highSize=%d nBatch=%d batchSize=%d lastBatchSize=%d\n"), // # nocov + nrow, ngrp, nb, bitshift, highSize, nBatch, batchSize, lastBatchSize); // # nocov } // initial population of g: #pragma omp parallel for num_threads(getDTthreads(ngrp, false)) @@ -108,9 +108,9 @@ SEXP gforce(SEXP env, SEXP jsub, SEXP o, SEXP f, SEXP l, SEXP irowsArg) { const int *restrict op = INTEGER(o); // o is a permutation of 1:nrow int nb = nbit(nrow-1); - int shift = MAX(nb-8, 0); // TODO: experiment nb/2. Here it doesn't have to be /2 currently. - int highSize = ((nrow-1)>>shift) + 1; - //Rprintf(_("When assigning grp[o] = g, highSize=%d nb=%d shift=%d nBatch=%d\n"), highSize, nb, shift, nBatch); + int bitshift = MAX(nb-8, 0); // TODO: experiment nb/2. Here it doesn't have to be /2 currently. + int highSize = ((nrow-1)>>bitshift) + 1; + //Rprintf(_("When assigning grp[o] = g, highSize=%d nb=%d bitshift=%d nBatch=%d\n"), highSize, nb, bitshift, nBatch); int *counts = calloc(nBatch*highSize, sizeof(int)); // TODO: cache-line align and make highSize a multiple of 64 int *TMP = malloc(nrow*2l*sizeof(int)); // must multiple the long int otherwise overflow may happen, #4295 if (!counts || !TMP ) error(_("Internal error: Failed to allocate counts or TMP when assigning g in gforce")); @@ -120,7 +120,7 @@ SEXP gforce(SEXP env, SEXP jsub, SEXP o, SEXP f, SEXP l, SEXP irowsArg) { const int *my_o = op + b*batchSize; int *restrict my_counts = counts + b*highSize; for (int i=0; i> shift; + const int w = (my_o[i]-1) >> bitshift; my_counts[w]++; } for (int i=0, cum=0; i> shift; // could use my_high but may as well use my_pg since we need my_pg anyway for the lower bits next too + const int w = (my_o[i]-1) >> bitshift; // could use my_high but may as well use my_pg since we need my_pg anyway for the lower bits next too int *p = my_tmp + 2*my_counts[w]++; *p++ = my_o[i]-1; *p = my_g[i]; @@ -172,7 +172,7 @@ SEXP gforce(SEXP env, SEXP jsub, SEXP o, SEXP f, SEXP l, SEXP irowsArg) { const int *my_pg = gp + b*batchSize; const int howMany = b==nBatch-1 ? lastBatchSize : batchSize; for (int i=0; i> shift; + const int w = my_pg[i] >> bitshift; my_counts[w]++; my_high[i] = (uint16_t)w; // reduce 4 bytes to 2 } @@ -185,7 +185,7 @@ SEXP gforce(SEXP env, SEXP jsub, SEXP o, SEXP f, SEXP l, SEXP irowsArg) { int *restrict my_tmpcounts = tmpcounts + omp_get_thread_num()*highSize; memcpy(my_tmpcounts, my_counts, highSize*sizeof(int)); for (int i=0; i> shift; // could use my_high but may as well use my_pg since we need my_pg anyway for the lower bits next too + const int w = my_pg[i] >> bitshift; // could use my_high but may as well use my_pg since we need my_pg anyway for the lower bits next too my_low[my_tmpcounts[w]++] = (uint16_t)(my_pg[i] & mask); } // counts is now cumulated within batch (with ending values) and we leave it that way @@ -362,7 +362,7 @@ SEXP gsum(SEXP x, SEXP narmArg) if (!anyNA) { #pragma omp parallel for num_threads(getDTthreads(highSize, false)) //schedule(dynamic,1) for (int h=0; h b ? a : b; } -void initDTthreads() { +void initDTthreads(void) { // called at package startup from init.c // also called by setDTthreads(threads=NULL) (default) to reread environment variables; see setDTthreads below // No verbosity here in this setter. Verbosity is in getDTthreads(verbose=TRUE) @@ -169,16 +169,16 @@ SEXP setDTthreads(SEXP threads, SEXP restore_after_fork, SEXP percent, SEXP thro static int pre_fork_DTthreads = 0; -void when_fork() { +void when_fork(void) { pre_fork_DTthreads = DTthreads; DTthreads = 1; } -void after_fork() { +void after_fork(void) { if (RestoreAfterFork) DTthreads = pre_fork_DTthreads; } -void avoid_openmp_hang_within_fork() { +void avoid_openmp_hang_within_fork(void) { // Called once on loading data.table from init.c #ifdef _OPENMP pthread_atfork(&when_fork, &after_fork, NULL); diff --git a/src/snprintf.c b/src/snprintf.c index 94199af70..6b8098c6f 100644 --- a/src/snprintf.c +++ b/src/snprintf.c @@ -184,7 +184,7 @@ int dt_win_snprintf(char *dest, const size_t n, const char *fmt, ...) return nc; } -SEXP test_dt_win_snprintf() +SEXP test_dt_win_snprintf(void) { char buff[50]; diff --git a/src/utils.c b/src/utils.c index 9d6f5d759..fa10fd97c 100644 --- a/src/utils.c +++ b/src/utils.c @@ -370,7 +370,7 @@ SEXP coerceAs(SEXP x, SEXP as, SEXP copyArg) { #ifndef NOZLIB #include #endif -SEXP dt_zlib_version() { +SEXP dt_zlib_version(void) { char out[71]; #ifndef NOZLIB snprintf(out, 70, "zlibVersion()==%s ZLIB_VERSION==%s", zlibVersion(), ZLIB_VERSION);