[Rd] Change to r-devel warns on #pragma

[Patrick Perry]

Dear Mark,

Thank you for the reply. I am not questioning the utility of compiler 
warnings in general. If you look at the code snippet, you can see that I 
am only disabling one warning ("-Wtype-limits"), in one line of code. 
This particular warning is spurious, so I want to ignore it, but I want 
warnings to be active for the rest of the project.

The specific issue is that I have a dynamic array of integers with an 
int length. When I grow the array, I want to make sure that the size 
doesn't overflow a size_t. I have an overflow check of the form

int n; // array size

if ((size_t)n > SIZE_MAX / sizeof(int)) {
    // handle overflow
}

The condition will never be true on a 64-bit architecture with 
sizeof(int) == 4 and sizeof(size_t) == 8 but it might be true on a 
32-bit architecture with sizeof(int) == sizeof(size_t). When compiled on 
a 64-bit architecture with "-Wextra", gcc emits a warning. Clang does 
not. I added the pragmas to disable this warning.

You might think that I could fix things by doing something like

#if sizeof(size_t) == sizeof(int)
/// check for overflow
#endif

but that won't work, because you can't use sizeof in a static #if. The 
standard work-around is to use autoconf to define SIZEOF_SIZE_T and 
SIZEOF_INT as in 
https://stackoverflow.com/questions/1921295/x64-compatible-c-source , 
but I'd rather not add a dependency on autoconf.


Patrick
> Mark van der Loo <mailto:mark.vanderloo at gmail.com>
> December 11, 2017 at 3:42 PM
>
> Hi Patrick,
>
> It was recently added as a cran policy (thanks Dirk's cran policy 
> watch: https://twitter.com/markvdloo/status/935810241190617088).
>
> It seems to be a general stricter policy on keeping to the C(++) 
> standard. Warnings are there for a reason and should usually not be 
> ignored. I'm not familiar with the warning you are suppressing  but it 
> seems likely that your code might assume type size that is not 
> guaranteed by the standard and thus may differ a cross 
> systems/compilers. (An example is wchar_t which has typically 16b on 
> Windows as guaranteed by the standard and 32b on Unix)
>
> Best,
> Mark
>
>
> Patrick Perry <mailto:pperry at stern.nyu.edu>
> December 11, 2017 at 10:32 AM
> A recent change to r-devel causes an R CMD check warning when a C file 
> includes a "#pragma GCC diagnostic ignored" pragma: 
> https://github.com/wch/r-source/commit/b76c8fd355a0f5b23d42aaf44a879cac0fc31fa4 
> . This causes the CRAN checks for the "corpus" package to emit a 
> warning: 
> https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-fedora-clang/corpus-00check.html 
> .
>
> The offending code is in an upstream library bundled with the package: 
> https://github.com/patperry/corpus/blob/master/src/table.c#L118
>
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wtype-limits"
>         // gcc emits a warning if sizeof(size_t) > sizeof(unsigned)
>
>         if ((size_t)size > SIZE_MAX / sizeof(*items)) {
> #pragma GCC diagnostic pop
>
> This is code appears in the "corpus" library that gets bundled with 
> the corpus r-package but can also be installed by itself. I am the 
> maintainer for both projects but in theory the library is independent 
> from the r package (the latter depends on the former). I put the 
> pragma there in the first place because this is the cleanest way I 
> know of to remove the gcc compiler warning "comparison is always false 
> due to limited range of data type" which appears whenever 
> sizeof(unsigned) < sizeof(size_t); the warning does not appear for clang.
>
> Does anyone have recommendations for what I should do to remove the R 
> CMD check warning? Is it possible to do this while simultaneously 
> removing the gcc warning? Note that the package does not use autoconf.
>
> Fortunately, I am the maintainer for the included library, so I can 
> potentially remove the pragma. However, I can imagine that there are 
> many other cases of R packages bundling C libraries where R package 
> maintainers do not have control over the downstream source. Perhaps 
> there is a compelling case for this new CRAN check that I'm not 
> seeing, but it seems to me that this additional CRAN check will cause 
> extra work for package developers without providing any additional 
> safety for R users. Package developers that do not control bundled 
> upstream libraries will have to resort to `sed s/^#pragma.*//` or 
> manually patch unfamiliar code to remove the CRAN warning, potentially 
> introducing bugs in the process.
>
>
> Patrick


	[[alternative HTML version deleted]]