Rdatatable / data.table

R's data.table package extends data.frame:
http://r-datatable.com
Mozilla Public License 2.0
3.62k stars 985 forks source link

Poor encapsulation in fread/fwrite headers #6468

Open MichaelChirico opened 2 months ago

MichaelChirico commented 2 months ago

Related to #6466. Note the current compiler warnings:

freadR.h:24:13: warning: ‘internal_error_buff’ defined but not used [-Wunused-variable]
   24 | static char internal_error_buff[1001]; // match internalErrSize
      |             ^~~~~~~~~~~~~~~~~~~
In file included from fwriteR.c:3:
fwrite.h:12:15: warning: ‘internal_error_buff’ defined but not used [-Wunused-variable]
   12 |   static char internal_error_buff[256];
      |               ^~~~~~~~~~~~~~~~~~~

For fwrite.h, the issue is INTERNAL_STOP() is never used in fwriteR.c, only in fwrite.c:

https://github.com/Rdatatable/data.table/blob/f248bbe6d1204dfc8def62328788eaadcc8e17a1/src/fwrite.c#L673

But fwrite.h is included in both files:

https://github.com/Rdatatable/data.table/blob/f248bbe6d1204dfc8def62328788eaadcc8e17a1/src/fwriteR.c#L3

IMO this is sloppy design, but it's a bit hard to unentangle -- requires a careful accounting of what's used where across the two .c files.

philippechataignon commented 2 months ago

In fwrite.c, INTERNAL_STOP() can be replaced by STOP(). Then internal_error_buff and INTERNAL_STOP can be removed from fwrite.h.

Patch :


diff --git a/src/fwrite.c b/src/fwrite.c
index 4229fab5..62f80bbb 100644
--- a/src/fwrite.c
+++ b/src/fwrite.c
@@ -684,7 +684,7 @@ void fwriteMain(fwriteMainArgs args)
         width = getMaxListItemLen(args.columns[j], args.nrow);
         break;
       default:
-        INTERNAL_STOP("type %d has no max length method implemented", args.whichFun[j]);  // # nocov
+        STOP(_("type %d has no max length method implemented"), args.whichFun[j]);  // # nocov
       }
     }
     if (args.whichFun[j] == WF_Float64 && args.scipen > 0)
diff --git a/src/fwrite.h b/src/fwrite.h
index 2dab5808..b50666fa 100644
--- a/src/fwrite.h
+++ b/src/fwrite.h
@@ -9,8 +9,6 @@
   #include "po.h"
   #define STOP     error
   #define DTPRINT  Rprintf
-  static char internal_error_buff[256];
-  #define INTERNAL_STOP(...) do {snprintf(internal_error_buff, 255, __VA_ARGS__); error("%s %s: %s. %s", _("Internal error in"), __func__, internal_error_buff, _("Please report to the data.table issues tracker"));} while (0)
 #endif

 typedef void writer_fun_t(const void *, int64_t, char **);
tdhock commented 2 months ago

I see these warnings using both R CMD INSTALL and cc(), on windows with rtools44