Rcpp: define STRICT_R_HEADERS to avoid definition of Free, ...
When this macro is not defined, R headers do define macros with very generic names, such as Free for S-PLUS back compatibility:
https://github.com/wch/r-source/blob/trunk/src/include/R_ext/RS.h#L70
#ifndef STRICT_R_HEADERS
/* S-PLUS 3.x but not 5.x NULLs the pointer in the following */
#define Calloc(n, t) (t *) R_chk_calloc( (R_SIZE_T) (n), sizeof(t) )
#define Realloc(p,n,t) (t *) R_chk_realloc( (void *)(p), (R_SIZE_T)((n) * sizeof(t)) )
#define Free(p) (R_chk_free( (void *)(p) ), (p) = NULL)
#endif
https://github.com/wch/r-source/blob/trunk/src/include/R_ext/RS.h#L49
#ifndef STRICT_R_HEADERS
#define R_PROBLEM_BUFSIZE 4096
/* Parentheses added for FC4 with gcc4 and -D_FORTIFY_SOURCE=2 */
#define PROBLEM {char R_problem_buf[R_PROBLEM_BUFSIZE];(sprintf)(R_problem_buf,
#define MESSAGE {char R_problem_buf[R_PROBLEM_BUFSIZE];(sprintf)(R_problem_buf,
#define ERROR ),error(R_problem_buf);}
#define RECOVER(x) ),error(R_problem_buf);}
#define WARNING(x) ),warning(R_problem_buf);}
#define LOCAL_EVALUATOR /**/
#define NULL_ENTRY /**/
#define WARN WARNING(NULL)
#endif
And https://github.com/wch/r-source/blob/trunk/src/include/R_ext/Constants.h#L35
#ifndef STRICT_R_HEADERS
#define PI M_PI
#include <float.h> /* Defines the rest, at least in C99 */
#define SINGLE_EPS FLT_EPSILON
#define SINGLE_BASE FLT_RADIX
#define SINGLE_XMIN FLT_MIN
#define SINGLE_XMAX FLT_MAX
#define DOUBLE_DIGITS DBL_MANT_DIG
#define DOUBLE_EPS DBL_EPSILON
#define DOUBLE_XMAX DBL_MAX
#define DOUBLE_XMIN DBL_MIN
#endif
For example the Free macro was making noise in arrow: https://github.com/romainfrancois/arrow/commit/79c50011db18db95c7cbee396d77a764e5c4b96d#diff-65a2f88d0196bb912b5ba669f5792492R20
So, should Rcpp have define STRICT_R_HEADERS somewhere ?
romainfrancois
All 34 comments
I am fairly certain we looked into this at some point early on when we did the header cleanup. And I am also pretty sure that I too had been bitten by it but I don't recall now why we are not doing it. That said, "it's late now" as we have 1400+ reverse depends so change may not be easy.
One way around it is of course to include Rcpp.h last but that is not always possible. Can you (for now) define STRICT_R_HEADERS before you include it for Arrow projects?
eddelbuettel
on 7 Sep 2018
Of course I can and I am. I don’t need this, i was just sharing the information.
romainfrancois
on 7 Sep 2018
And I started a reverse depends check to see how much breakage this may introduce. I am running a full "baseline" run first so this will take a moment.
eddelbuettel
on 7 Sep 2018
I'd be on-board with setting STRICT_R_HEADERS by default as long as there is not too much fallout from dependent packages.
kevinushey
on 7 Sep 2018
Yes. The non-"Namespace" (in quotes, because C ...) protected identifiers are a pain, and the clashes are a nuisance. We should promote better programming and packaging practice where possible.
We'll see what the rev.deps say, but at a minimum we could even try to phase it in.
eddelbuettel
on 7 Sep 2018
Would be less aggressive than undef them, Rcpp could offer an opt-in instead so that packages that really need these macros can still have them.
romainfrancois
on 7 Sep 2018
What I was thinking is that we could possibly #define STRICT_R_HEADERS with an option to opt-out, ie if you have something set, say RCPP_NO_STRICT_HEADERS then we won't.
But let's first see what damage (if any) a definition may cause.
eddelbuettel
on 7 Sep 2018
Does not look too good. Baseline run against Rcpp 0.12.18 (once I move two new ones to skip):
Test of Rcpp had 1357 successes, 16 failures, and 60 skipped packages.
where 'the usual' set of packages fails.
But with STRICT_R_HEADERS:
Test of unknown had 1295 successes, 78 failures, and 60 skipped packages.
Details in https://github.com/RcppCore/rcpp-logs/commit/f05b3e42a769be676e9130de39ea6089c07404fd
So for the time being this may be a documentation-only thing.
Edited: Had a copy and paste error reporting the initial run twice.
eddelbuettel
on 8 Sep 2018
Dang, that's more than I expected 😞
Given the number of failures, I don't think we should force the issue -- client packages can (as arrow does) just provide the define themselves, e.g.
#define STRICT_R_HEADERS
#include <Rcpp.h>
(or perhaps even better enforce in PKG_CPPFLAGS from Makevars)
kevinushey
on 8 Sep 2018
Yes and yes.
But I am thinking we should still try to get this in. I was thinking of two approaches, the second idea is a little fresher and less mature :grinning:
First Approach: Contact Maintainers
This is old gum-shoe work. Email all 60+ maintainers. Ask for a change. Give a reasonable window, say one year.
Second Approach: configure !!
Given that we know which packages (or CRAN, among non-skipped ones) fail, we could have configure add / not-add the #define conditionally. For the currently-failing packages we do not set it, but issue a loud warning. We also document "everywhere" that you can opt out with a RCPP_NO_STRICT_HEADERS definition.
What do you think? Is this feasible?
eddelbuettel
on 8 Sep 2018
Using the SQLite result files from the prrd runs committed to the sibbling rcpp-logs repo:
suppressMessages({
library(RSQLite)
library(data.table)
})
run1 <- dbConnect(SQLite(), "~/git/rcpp-logs/queues/Rcpp_2018-09-07/queuefile.sqlite")
run2 <- dbConnect(SQLite(), "~/git/rcpp-logs/queues/Rcpp_2018-09-08/queuefile.sqlite")
res1 <- setDT(dbGetQuery(run1, "select * from results"))
res2 <- setDT(dbGetQuery(run2, "select * from results"))
## correct res1 for the two which were later added to skip list
res1[ package %in% c("bgsmtr", "EstMix") , result:=2]
## find the ones in run2 that failed
cands <- res2[ result==1, 1:3 ]
## inner join with run1, extract the ones that worked there --> new failures
fails <- res1[cands[,1:3], 1:3, on="package"][result==0]
gets us
R> fails
package version result
1: basad 0.2.0 0
2: bayesImageS 0.5-3 0
3: bcp 4.0.3 0
4: bfa 0.4 0
5: biglasso 1.3-6 0
6: bigmemory 4.5.33 0
7: bigReg 0.1.2 0
8: binnednp 0.1.0 0
9: biwavelet 0.20.17 0
10: blockmodels 1.1.1 0
11: castor 1.3.4 0
12: cbinom 1.3 0
13: circumplex 0.1.2 0
14: CorReg 1.2.8 0
15: CoxPlus 1.1.1 0
16: DGM 1.7.2 0
17: emIRT 0.0.8 0
18: EMVS 1.0 0
19: ETAS 0.4.4 0
20: facilitation 0.5.2 0
21: fasteraster 1.1.1 0
22: fastGHQuad 0.2 0
23: extraDistr 1.8.9 0
24: fDMA 2.2.3.1 0
25: frailtySurv 1.3.5 0
26: gcKrig 1.1.2 0
27: GAS 0.2.8 0
28: geogrid 0.1.0.1 0
29: glmBfp 0.0-51 0
30: Gmisc 1.6.2 0
31: HardyWeinberg 1.6.1 0
32: icenReg 2.0.8 0
33: lidR 1.6.1 0
34: lpme 1.1.1 0
35: MAINT.Data 1.2.3 0
36: mcmcse 1.3-2 0
37: medfate 0.2.2 0
38: meteoland 0.7.5 0
39: minimaxdesign 0.1.3 0
40: MSGARCH 2.3 0
41: multdyn 1.6 0
42: MultiFit 0.1.2 0
43: mwaved 1.1.5 0
44: netdiffuseR 1.20.0 0
45: nngeo 0.2.1 0
46: npsf 0.4.1 0
47: packcircles 0.3.2 0
48: PAFit 1.0.0.6 0
49: partialCI 1.1.1 0
50: pense 1.2.0 0
51: precrec 0.9.1 0
52: recmap 0.5.37 0
53: ReIns 1.0.7 0
54: rmgarch 1.3-0 0
55: robustlmm 2.2-1 0
56: rococo 1.1.5 0
57: rotations 1.5 0
58: rtdists 0.9-0 0
59: serrsBayes 0.3-13 0
60: unmarked 0.12-2 0
61: updog 1.0.1 0
62: vegclust 1.7.4 0
package version result
R>
or
R> dput(fails[,1:2])
structure(list(package = c("basad", "bayesImageS", "bcp", "bfa",
"biglasso", "bigmemory", "bigReg", "binnednp", "biwavelet", "blockmodels",
"castor", "cbinom", "circumplex", "CorReg", "CoxPlus", "DGM",
"emIRT", "EMVS", "ETAS", "facilitation", "fasteraster", "fastGHQuad",
"extraDistr", "fDMA", "frailtySurv", "gcKrig", "GAS", "geogrid",
"glmBfp", "Gmisc", "HardyWeinberg", "icenReg", "lidR", "lpme",
"MAINT.Data", "mcmcse", "medfate", "meteoland", "minimaxdesign",
"MSGARCH", "multdyn", "MultiFit", "mwaved", "netdiffuseR", "nngeo",
"npsf", "packcircles", "PAFit", "partialCI", "pense", "precrec",
"recmap", "ReIns", "rmgarch", "robustlmm", "rococo", "rotations",
"rtdists", "serrsBayes", "unmarked", "updog", "vegclust"), version = c("0.2.0",
"0.5-3", "4.0.3", "0.4", "1.3-6", "4.5.33", "0.1.2", "0.1.0",
"0.20.17", "1.1.1", "1.3.4", "1.3", "0.1.2", "1.2.8", "1.1.1",
"1.7.2", "0.0.8", "1.0", "0.4.4", "0.5.2", "1.1.1", "0.2", "1.8.9",
"2.2.3.1", "1.3.5", "1.1.2", "0.2.8", "0.1.0.1", "0.0-51", "1.6.2",
"1.6.1", "2.0.8", "1.6.1", "1.1.1", "1.2.3", "1.3-2", "0.2.2",
"0.7.5", "0.1.3", "2.3", "1.6", "0.1.2", "1.1.5", "1.20.0", "0.2.1",
"0.4.1", "0.3.2", "1.0.0.6", "1.1.1", "1.2.0", "0.9.1", "0.5.37",
"1.0.7", "1.3-0", "2.2-1", "1.1.5", "1.5", "0.9-0", "0.3-13",
"0.12-2", "1.0.1", "1.7.4")), class = c("data.table", "data.frame"
), row.names = c(NA, -62L), .internal.selfref = <pointer: 0x55a6d42ba0b0>)
R>
I'm not a fan of just screening these packages in configure unless we give these packages a way to communicate to Rcpp that they now do indeed compile with that define somehow. Overall that route seems not worth the trouble.
"Contact the maintainers" seems a better way forward. We might even consider a brief message to CRAN (Uwe) just to see if they agree the change would be warranted given the potential for a bit of disruption. (This seems like the sort of thing CRAN might want to enforce globally some day)
kevinushey
on 10 Sep 2018
Yes, screening is kinda hackish. Another attempt may be to email all maintainers now, say we plan to do this "in due course" but with suitable warning, say in 12 months, and then revisit in 3, 6, 0, ... months as we have time. Hopefully the list whittles down...
eddelbuettel
on 10 Sep 2018
New plan. I will email all 50+ maintainers, explaining that their package failed with strict headers and asking that they update within the year. I will also email rcpp-devel.
And come September 2019, we add the #define. Better?
Code
With fails as above, adding
db <- setDT(tools::CRAN_package_db())
maints <- db[ fails[, .(package, version)], .(Package, Version, Maintainer) ]
## need one special case: one package without accent in name, two with
maints[Maintainer == "Miquel De Caceres <[email protected]>",
Maintainer := "Miquel De Cáceres <[email protected]>" ]
setkey(maints, Maintainer)
maints[ , .(pkgs=paste(Package, collapse=", ")), by=Maintainer]
get us these maintainers:
R> maints[ , .(pkgs=paste(Package, collapse=", ")), by=Maintainer]
Maintainer pkgs
1: Abdollah Jalilian <[email protected]> ETAS
2: Alexander W Blocker <[email protected]> fastGHQuad
3: Alexios Ghalanos <[email protected]> rmgarch
4: Andy Bosyi <[email protected]> fasteraster
5: Andy Royle <[email protected]> unmarked
6: Bryan Stanfill <[email protected]> rotations
7: Chibisi Chima-Okereke <[email protected]> bigReg
8: Christian Panse <[email protected]> recmap
9: Clement THERY <[email protected]> CorReg
10: Clifford Anderson-Bergman <[email protected]> icenReg
11: Dan Dalthorp <[email protected]> cbinom
12: Daniel Barreiro Ures <[email protected]> binnednp
13: David Gerard <[email protected]> updog
14: David Kepplinger <[email protected]> pense
15: Dootika Vats <[email protected]> mcmcse
16: Gemma Moran <[email protected]> EMVS
17: George Vega Yon <[email protected]> netdiffuseR
18: Haiming Zhou <[email protected]> lpme
19: Henrik Singmann <[email protected]> rtdists
20: Isaac Gravestock <[email protected]> glmBfp
21: James Lo <[email protected]> emIRT
22: Jan Graffelman <[email protected]> HardyWeinberg
23: Jared Murray <[email protected]> bfa
24: Jean-Benoist Leger <[email protected]> blockmodels
25: Jean-Romain Roussel <[email protected]> lidR
26: Jeffrey Girard <[email protected]> circumplex
27: Jing Peng <[email protected]> CoxPlus
28: Jonas Rende <[email protected]> partialCI
29: Justin Rory Wishart <[email protected]> mwaved
30: Keven Bluteau <[email protected]> MSGARCH
31: Krzysztof Drachal <[email protected]> fDMA
32: Leopoldo Catania <[email protected]> GAS
33: Mali Salles <[email protected]> facilitation
34: Manuel Koller <[email protected]> robustlmm
35: Matt Moores <[email protected]> bayesImageS, serrsBayes
36: Max Gordon <[email protected]> Gmisc
37: Michael Bedward <[email protected]> packcircles
38: Michael Dorman <[email protected]> nngeo
39: Michael J. Kane <[email protected]> bigmemory
40: Miquel De Cáceres <[email protected]> medfate, meteoland, vegclust
41: ORPHANED geogrid
42: Oleg Badunenko <[email protected]> npsf
43: Pedro Duarte Silva <[email protected]> MAINT.Data
44: Qingyan Xiang <[email protected]> basad
45: S. Gorsky <[email protected]> MultiFit
46: Simon Mak <[email protected]> minimaxdesign
47: Simon Schwab <[email protected]> DGM, multdyn
48: Stilianos Louca <[email protected]> castor
49: Takaya Saito <[email protected]> precrec
50: Tarik C. Gouhier <[email protected]> biwavelet
51: Thong Pham <[email protected]> PAFit
52: Tom Reynkens <[email protected]> ReIns
53: Tymoteusz Wolodzko <[email protected]> extraDistr
54: Ulrich Bodenhofer <[email protected]> rococo
55: Vinnie Monaco <[email protected]> frailtySurv
56: Xiaofei Wang <[email protected]> bcp
57: Yaohui Zeng <[email protected]> biglasso
58: Zifei Han <[email protected]> gcKrig
Maintainer pkgs
R>
Sounds good to me! Thanks for spearheading the effort.
kevinushey
on 16 Sep 2018
I wrote up something as basis for the email to rcpp-devel, CRAN and the maintainers which I just sent to rcpp-core (and CC to @romainfrancois). If you are reasing this here and there, please peruse.
Thanks!
eddelbuettel
on 16 Sep 2018
Sorry guys, maybe I'm stupid, but I replaced DBL_EPSILON in my package (rococo) by DOUBLE_EPS in conjunction with setting -DSTRICT_R_HEADERS in Makevars, but I get a compiler error that DOUBLE_EPS is not defined. Have I misunderstood something?
UBod
on 11 Oct 2018
I think it is the other way around: DBL_EPSILON is "good" and should be used, DOUBLE_EPS is sloppy and only here because of ancient S headers.
I just added #define STRICT_R_HEADERS so now it wants DBL_EPSILON and eg per this you want the limits.h (or climit) header.
The following diff makes it build and install for me. (As an aside, you still have a lot of SEXP around. You have not needed that for years. I find actual C++ types cleaner to work with.)
edd@rob:/tmp/rococo$ diff -ru rococo.orig/ rococo |head -40
diff -ru rococo.orig/src/rcor_bm.h rococo/src/rcor_bm.h
--- rococo.orig/src/rcor_bm.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_bm.h 2018-10-11 09:35:25.110306046 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_BM_H
#define _RCOR_BM_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
RcppExport SEXP rcor_matrix_classical(SEXP vx, SEXP r);
diff -ru rococo.orig/src/rcor.cpp rococo/src/rcor.cpp
--- rococo.orig/src/rcor.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor.cpp 2018-10-11 09:35:07.942507260 -0500
@@ -1,3 +1,4 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
#include "tnorms.h"
#include "rcor.h"
diff -ru rococo.orig/src/rcor_exacttest.cpp rococo/src/rcor_exacttest.cpp
--- rococo.orig/src/rcor_exacttest.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_exacttest.cpp 2018-10-11 09:39:52.815183485 -0500
@@ -1,4 +1,6 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
+#include <limits>
#include "tnorms.h"
#include "rcor.h"
@@ -139,7 +141,7 @@
do
{
get_sums(mat_x, mat_y, perm, tnorm_fp, &c, &d);
- gamma[0] = (fabs(c + d) <= DBL_EPSILON ? 0 : (c - d) / (c + d));
+ gamma[0] = (fabs(c + d) <= std::numeric_limits<double>::epsilon() ? 0 : (c - d) / (c + d));
if (numGamma) permGamma[i] = gamma[0];
diff -ru rococo.orig/src/rcor_exacttest.h rococo/src/rcor_exacttest.h
--- rococo.orig/src/rcor_exacttest.h 2017-07-04 12:43:02.000000000 -0500
--- rococo.orig/src/rcor_exacttest.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_exacttest.h 2018-10-11 09:35:36.466173026 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_EXACTTEST_H
#define _RCOR_EXACTTEST_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
using namespace Rcpp;
diff -ru rococo.orig/src/rcor.h rococo/src/rcor.h
--- rococo.orig/src/rcor.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor.h 2018-10-11 09:35:44.330080946 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_H
#define _RCOR_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
using namespace Rcpp;
diff -ru rococo.orig/src/rcor_permtest.cpp rococo/src/rcor_permtest.cpp
--- rococo.orig/src/rcor_permtest.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_permtest.cpp 2018-10-11 09:41:16.726209321 -0500
@@ -1,4 +1,6 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
+#include <limits>
#include "tnorms.h"
#include "rcor.h"
#include "rcor_permtest.h"
@@ -66,7 +68,7 @@
shuffle_in_place(perm);
get_sums(mat_x, mat_y, perm, tnorm_fp, &c, &d);
- gamma[0] = (fabs(c + d) <= DBL_EPSILON) ? 0 : (c - d) / (c + d);
+ gamma[0] = (fabs(c + d) <= std::numeric_limits<double>::epsilon()) ? 0 : (c - d) / (c + d);
if (numGamma) permGamma[i] = gamma[0];
diff -ru rococo.orig/src/rcor_permtest.h rococo/src/rcor_permtest.h
--- rococo.orig/src/rcor_permtest.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_permtest.cpp 2018-10-11 09:41:16.726209321 -0500
@@ -1,4 +1,6 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
+#include <limits>
#include "tnorms.h"
#include "rcor.h"
#include "rcor_permtest.h"
@@ -66,7 +68,7 @@
shuffle_in_place(perm);
get_sums(mat_x, mat_y, perm, tnorm_fp, &c, &d);
- gamma[0] = (fabs(c + d) <= DBL_EPSILON) ? 0 : (c - d) / (c + d);
+ gamma[0] = (fabs(c + d) <= std::numeric_limits<double>::epsilon()) ? 0 : (c - d) / (c + d);
if (numGamma) permGamma[i] = gamma[0];
diff -ru rococo.orig/src/rcor_permtest.h rococo/src/rcor_permtest.h
--- rococo.orig/src/rcor_permtest.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_permtest.h 2018-10-11 09:35:54.513961738 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_PERMTEST_H
#define _RCOR_PERMTEST_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
using namespace Rcpp;
edd@rob:/tmp/rococo$
Thanks a lot, Dirk, that solved the issue! I will upload a new version to CRAN in due course.
Regarding your remark about the use of SEXP: yes, I am aware of this. I am actually using C++ types in my newer work, but I currently do not have the time to update my older packages as long as they still work well.
UBod
on 11 Oct 2018
Great! And sorry for the false lead earlier. I too thought the define was present was seemingly not. But the numeric_limits<double>::epsilon() is definitely cooler and the way to go. And no worries about old(er) code.
eddelbuettel
on 11 Oct 2018
I just revisited this issue with two full reverse depends checks from current HEAD:
- 19 failures out of 1692 package (module some skipped ones) for HEAD
- 81 failures once we add R_STRICT_HEADERS
So this still seems like a no-go despite the excellent help by those who made changes. But 62 affected packages is not something we can swing by CRAN I am afraid.
Details here: https://github.com/RcppCore/rcpp-logs/commit/441af5d85ad7b4644314224d028b6bcf79815c17
eddelbuettel
on 6 Jul 2019
I am a newbie to the Rcpp world and I ran into this issue with Free. It wasn't too hard to figure the issue out. I saw R_STRICT_HEADERS and just added it and it fixed my issue. It would be great to have this as the default to lower the burden on new Rcpp users, but I understand the compatibility issue.
dlong11
on 22 Apr 2020
It looks like we had planned to switch to STRICT_R_HEADERS a while back:
but that transition wasn't made due to breakage in downstream packages. Maybe worth considering for a future Rcpp release (ideally after all the issues with the R 4.0.0 release have been ironed out)?
kevinushey
on 22 Apr 2020
Oh, if someone wants to volunteer testing this, coordinating will all affected downstream packages as well as CRAN ... then I surely won't stand in the way. But I am also unlikely to lead it anytime soon.
eddelbuettel
on 22 Apr 2020
I revisited the issue of possibly adding STRICT_R_HEADERS and ran two reverse dependency runs:
- a 'baseline' with the current mainline branch Rcpp 1.0.6.6 (results summarised here)
- a minimal change of just defining
STRICT_R_HEADERSat the top ofRcpp.h, I called that Rcpp 1.0.6.6.1 as a variant (result summarised here)
The net is that we are now looking at about 52 failures, as opposed to 62 when we ran in July 2019 (see comment above).
I would like to have this on. I think we could (slowly) tackle providing PRs to 50-some package. The bigger question is: 50-some out 2260. Is that enough cause for concern regarding code not on CRAN that might break? Or is it a risk worth taking?
eddelbuettel
on 8 Apr 2021
Maybe we could add this define when COMPILING_RCPPand otherwise start emitting a big warning for some time if STRICT_R_HEADERS is not defined?
Enchufa2
on 8 Apr 2021
That is a good idea, but what bugs me a little about it is that we have several classes:
- users who not define it but should have defined it as they use
PIinstead ofM_PIorERRORand so one -- the 'hard fail' of the 52 failures above finds them - users who do not define it and don't use one of the symbols (I'd venture the majority of people) and who would get a needless
WARNINGfor not having defined something ... they don't need - users who define it and use it: no issue, no change
- users who define but don't use it: nobody is bothered
So in order to get to the first group we also yell at the second group. Can we do better?
eddelbuettel
on 24 Apr 2021
I think that the only leverage point we have is the moment in which RcppExports are created. Rcpp could then scan the code for cases in the first group and emit that warning only for them. After some time, some releases, that warning could become an error.
Enchufa2
on 24 Apr 2021
Here's an example of what I'm proposing: https://github.com/RcppCore/Rcpp/commit/222a17bd1303e3fbf6736da6e13998d32c2473f8. Running Rcpp::compileAttributes e.g. on the basad package shows the following with this patch:
Warning message:
Some src files have incompatibilities with STRICT_R_HEADERS,
which will be the default in future versions of Rcpp (see
https://github.com/RcppCore/Rcpp/issues/898 for more information)
Please, replace the following macros:
- File src/utiliti.cpp: PI
The message can be improved, but that's the idea: a warning with a link to this issue and a list of files and macros to replace.
Enchufa2
on 25 Apr 2021
Having it a phase-in is a nice approach. We could even have it as a message for one release (~ six months), then a warning for one release (~ six months) and then turn to an error.
Any thoughts on how hard it would be to do the same things via std::basic_regex() in compiled code? I haven't looked in a while but there should a number of other (compiled code, pre-compiled regular expression) examples in the guts of the attribut es code.
@jjallaire Any word of wisdom as to whether it would foolish (:disappointed: ) or feasible (:smiley: ) to do this? Having support for STRICT_R_HEADERS enabled would be a "Good Thing (TM)".
eddelbuettel
on 25 Apr 2021
I wouldn't do anything with regex's here.
I do however think it would be good to take the approach you suggested with the additional step of emailing the maintainers now, in 6 months, and then again in 12 months. My guess is we'd get ~ 50/62 packages on board in that timeframe, and then given this level of effort by us CRAN might find the change more acceptable.
jjallaire
on 25 Apr 2021
Yes, that was my original plan -- but I got a little distracted but I think I will just sit down and open a new ticket with the list of 50 to 60 packages I had, and then between us (and any volunteers, hah!) we can pick those off and prepare a (simple) PR every couple of days (for packages on GH). Coupled with the usual emails. We can take the long view here, it doesn't have to be done by this summer but it would be great to get it in two cycles. Thanks thanks for the well-reasoned thumbs-up.
Having Inaki's commit may still help for a run on the test machine to get clearly identified error messages.
eddelbuettel
on 25 Apr 2021
That sounds right to me!
jjallaire
on 25 Apr 2021
Pass one complete. 51 potential changes turned to 50 (as one apparently had been addressed in the meantime or didn't reproduce). All fifty have gotten a PR, or a patch via email, and seventeen have already responded.
eddelbuettel
on 8 May 2021
Related issues
kevinushey
·
6Comments
pkrog
·
5Comments
karthik2527
·
4Comments
epipping
·
7Comments
wch
·
7Comments
Most helpful comment
New plan. I will email all 50+ maintainers, explaining that their package failed with strict headers and asking that they update within the year. I will also email
rcpp-devel.And come September 2019, we add the
#define. Better?Code
With
failsas above, addingget us these maintainers: