Rdatatable / data.table

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

gcc on windows warns for unnamed structs under cc() but not R CMD INSTALL #6471

Closed tdhock closed 1 month ago

tdhock commented 1 month ago

related to #6466 and #6468 which I observe also.

on windows using gcc/rtools44 I see no warning when compiling reorder.c (for example, other are affected too)

$ R CMD INSTALL .
* installing to library 'C:/Users/hoct2726/AppData/Local/R/win-library/4.4'
* installing *source* package 'data.table' ...
** using staged installation

   **********************************************
   WARNING: this package has a configure script
         It probably needs manual configuration
   **********************************************

** libs
using C compiler: 'gcc.exe (GCC) 13.2.0'
...
gcc  -I"C:/PROGRA~1/R/R-44~1.1/include" -DNDEBUG     -I"C:/rtools44/x86_64-w64-mingw32.static.posix/include"  -fopenmp   -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign  -c reorder.c -o reorder.o
gcc  -I"C:/PROGRA~1/R/R-44~1.1/include" -DNDEBUG     -I"C:/rtools44/x86_64-w64-mingw32.static.posix/include"  -fopenmp   -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign  -c shift.c -o shift.o

with cc(), I see a warning:

> cc()
C:/Users/hoct2726/R/data.table/src 
Running command:
MAKEFLAGS='-j CC=gcc CFLAGS=-fopenmp\ -std=c99\ -O3\ -pipe\ -Wall\ -pedantic\ -Wstrict-prototypes\ -isystem\ /usr/share/R/include\ -DWIN32\ -fno-common' R CMD SHLIB -o data_table.so *.c
using C compiler: 'gcc.exe (GCC) 13.2.0'
gcc -I"C:/PROGRA~1/R/R-44~1.1/include" -DNDEBUG     -I"C:/rtools44/x86_64-w64-mingw32.static.posix/include"  -fopenmp   -fopenmp -std=c99 -O3 -pipe -Wall -pedantic -Wstrict-prototypes -isystem /usr/share/R/include -DWIN32 -fno-common -c assign.c -o assign.o
gcc -I"C:/PROGRA~1/R/R-44~1.1/include" -DNDEBUG     -I"C:/rtools44/x86_64-w64-mingw32.static.posix/include"  -fopenmp   -fopenmp -std=c99 -O3 -pipe -Wall -pedantic -Wstrict-prototypes -isystem /usr/share/R/include -DWIN32 -fno-common -c between.c -o between.o
...
gcc -I"C:/PROGRA~1/R/R-44~1.1/include" -DNDEBUG     -I"C:/rtools44/x86_64-w64-mingw32.static.posix/include"  -fopenmp   -fopenmp -std=c99 -O3 -pipe -Wall -pedantic -Wstrict-prototypes -isystem /usr/share/R/include -DWIN32 -fno-common -c reorder.c -o reorder.o
In file included from C:/PROGRA~1/R/R-44~1.1/include/R.h:71,
                 from data.table.h:3,
                 from nafill.c:1:
C:/PROGRA~1/R/R-44~1.1/include/R_ext/Complex.h:80:6: warning: ISO C99 doesn't support unnamed structs/unions [-Wpedantic]
   80 |     };
      |      ^
gcc -I"C:/PROGRA~1/R/R-44~1.1/include" -DNDEBUG     -I"C:/rtools44/x86_64-w64-mingw32.static.posix/include"  -fopenmp   -fopenmp -std=c99 -O3 -pipe -Wall -pedantic -Wstrict-prototypes -isystem /usr/share/R/include -DWIN32 -fno-common -c shift.c -o shift.o

The different/unique compiler flags are:

R CMD INSTALL
-O2  -mfpmath=sse -msse2 -mstackrealign

cc()
-fopenmp -std=c99 -O3 -pipe -pedantic -Wstrict-prototypes -isystem /usr/share/R/include -DWIN32 -fno-common

I guess the issue comes from using -std=c99, I wonder if we can remove that? more generally, why are the cc() compiler flags so different from the R CMD INSTALL compiler flags? is that desirable for us for some reason? (If there is no strong reason for the difference, I would suggest changing cc flags to match R CMD INSTALL flags)

MichaelChirico commented 1 month ago

I've been ignoring this particular warning. It's explicitly noted in Complex.h:

https://github.com/r-devel/r-svn/blob/cf32314949a5ffeb5ea985367daa53db176ede79/src/include/R_ext/Complex.h#L49-L62

And yes I think it comes from -std=c99. As to that... it's been there since #2488. Before that it was -std=gnu99. That was documented as being done to reveal some issue:

https://github.com/Rdatatable/data.table/pull/2488/commits/98aa9d1b296c46a6ba77d93967e5e3d3fd9cf266

I agree it seems odd to have competing compilation options. If anything, maybe we should have different GHA testing "unusual" compiler setups.

MichaelChirico commented 1 month ago

Looking a bit further:

https://g.co/gemini/share/e7f3ab7b9f5a https://gcc.gnu.org/onlinedocs/gcc/Standards.html https://cran.r-project.org/doc/manuals/r-release/R-exts.html https://www.reddit.com/r/C_Programming/comments/7g7rol/what_std_do_you_use_with_gcc/ https://clang.llvm.org/c_status.html

Anirban166 commented 1 month ago

Specifying c11 or a higher standard should remove that warning.

I don't see what clang's default is.

Looks like gnu11 (or c11 with GNU extensions) on my Windows laptop:

clang -dM -E - < NUL | find "__STDC_VERSION__"
#define __STDC_VERSION__ 201112L

Off-topic but for comparison, clang (or rather Apple Clang) is gnu17 on my Mac:

clang -dM -E - < /dev/null | grep __STDC_VERSION__
#define __STDC_VERSION__ 201710L