Dear all, hoping this is the right place for this: I stumbled upon documentation regarding the use of `assert` in C++ code, in particular, https://cran.r-project.org/web/packages/policies.html states that ``` Thus C/C++ calls to assert/abort/exit/std::terminate, Fortran calls to STOP and so on must be avoided. ``` , which goes against all best practice guidelines I'm aware of. Therefore I would propose to remove 'assert' from the above statement. In support, https://cran.r-project.org/doc/manuals/r-release/R-exts.html states that ``` One usage that could call abort is the assert macro in C or C++ functions, which should never be active in production code. The normal way to ensure that is to define the macro NDEBUG, and R CMD INSTALL does so as part of the compilation flags. ``` which seems a reasonable thing to do. As a sidenote, the guide could be extended to encourage new mechanisms available in modern C++ like `static_assert`. Cheers Chris
[R-pkg-devel] use of assert in C++
7 messages · Dirk Eddelbuettel, Bielow, Chris, Duncan Murdoch
On 18 December 2024 at 19:46, Bielow, Chris wrote:
| hoping this is the right place for this: | I stumbled upon documentation regarding the use of `assert` in C++ code, in | particular, https://cran.r-project.org/web/packages/policies.html states | that | | ``` | Thus C/C++ calls to assert/abort/exit/std::terminate, Fortran calls to STOP | and so on must be avoided. | ``` | | , which goes against all best practice guidelines I'm aware of. Yes and no. Defensize programming is good, of course, but `assert()` leads to an `abort()` and immediate abort which for an _environment_ such as R is really not appropriate. You want a proper error return (i.e. Rf_error with a message or alike) to the R prompt. Note that you can always test something like this; there are several packages allowing ad-hoc compilation from `inline` to `Rcpp` and others. So here: > Rcpp::cppFunction(r"(void ohnoes() { assert(false); })", includes=c("#undef NDEBUG", "#include <assert.h>")) > ohnoes() R: file2cd1353560062b.cpp:8: void ohnoes(): Assertion `false' failed. Process R:3 aborted (core dumped) at Wed Dec 18 16:22:19 2024 ... and that took my R session down just like `assert()` is expected. Also note the trickery I had to do to re-enable `assert()` by undefining `NDEBUG` and including the header after that. So viewed from that angle, it really is not against _best practices for R extensions_ which is what the CRAN Policy is about. Cheers, Dirk | Therefore I would propose to remove 'assert' from the above statement. | In support, https://cran.r-project.org/doc/manuals/r-release/R-exts.html | states that | | ``` | One usage that could call abort is the assert macro in C or C++ functions, | which should never be active in production code. The normal way to ensure | that is to define the macro NDEBUG, and R CMD INSTALL does so as part of the | compilation flags. | ``` | which seems a reasonable thing to do. | | As a sidenote, the guide could be extended to encourage new mechanisms | available in modern C++ like `static_assert`. | | | Cheers | Chris | | ______________________________________________ | R-package-devel at r-project.org mailing list | https://stat.ethz.ch/mailman/listinfo/r-package-devel
dirk.eddelbuettel.com | @eddelbuettel | edd at debian.org
|| hoping this is the right place for this: || I stumbled upon documentation regarding the use of `assert` in C++ || code, in particular, || https://cran.r-project.org/web/packages/policies.html states that || || ``` || Thus C/C++ calls to assert/abort/exit/std::terminate, Fortran calls to || STOP and so on must be avoided. || ``` || || , which goes against all best practice guidelines I'm aware of. | |Yes and no. | |Defensize programming is good, of course, but `assert()` leads to an `abort()` and immediate abort which for an _environment_ such as R is really not appropriate. You want a proper error return (i.e. Rf_error with a message or alike) to the R prompt. | |Note that you can always test something like this; there are several packages allowing ad-hoc compilation from `inline` to `Rcpp` and others. So here: | | > Rcpp::cppFunction(r"(void ohnoes() { assert(false); })", includes=c("#undef NDEBUG", "#include <assert.h>")) | > ohnoes() | R: file2cd1353560062b.cpp:8: void ohnoes(): Assertion `false' failed. | | Process R:3 aborted (core dumped) at Wed Dec 18 16:22:19 2024 | |... and that took my R session down just like `assert()` is expected. Also note the trickery I had to do to re-enable `assert()` by undefining `NDEBUG` and including the header after that. Exactly! You had to jump through a lot of hoops to even make assert() crash your R session. This would _not_ happen in production code. So a developer of an R extension could happily use asserts, then use your method to test his/her code. No need to remove the asserts, since NDEBUG is defined in production. | |So viewed from that angle, it really is not against _best practices for R extensions_ which is what the CRAN Policy is about. I'm not following this argument, since you (in my eyes) actually made a point for the opposite (see above). Keeping in mind that R is used mostly for scientific purposes I'd favor correctness over inconenviences like a crashing R session (keeping in mind that it would not even crash as per the arguments above). Encouraging asserts() would be a more C++-like experience for the developer, resulting in safer, more robust code, which is a good thing. Just my 50ct. cheers Chris | |Cheers, Dirk | || Therefore I would propose to remove 'assert' from the above statement. || In support, || https://cran.r-project.org/doc/manuals/r-release/R-exts.html || states that || || ``` || One usage that could call abort is the assert macro in C or C++ || functions, which should never be active in production code. The normal || way to ensure that is to define the macro NDEBUG, and R CMD INSTALL || does so as part of the compilation flags. || ``` || which seems a reasonable thing to do. || || As a sidenote, the guide could be extended to encourage new mechanisms || available in modern C++ like `static_assert`. || || || Cheers || Chris || || ______________________________________________ || R-package-devel at r-project.org mailing list || https://stat.ethz.ch/mailman/listinfo/r-package-devel | |-- |dirk.eddelbuettel.com | @eddelbuettel | edd at debian.org |
On 2024-12-19 5:14 a.m., Bielow, Chris wrote:
|| hoping this is the right place for this: || I stumbled upon documentation regarding the use of `assert` in C++ || code, in particular, || https://cran.r-project.org/web/packages/policies.html states that || || ``` || Thus C/C++ calls to assert/abort/exit/std::terminate, Fortran calls to || STOP and so on must be avoided. || ``` || || , which goes against all best practice guidelines I'm aware of. | |Yes and no. | |Defensize programming is good, of course, but `assert()` leads to an `abort()` and immediate abort which for an _environment_ such as R is really not appropriate. You want a proper error return (i.e. Rf_error with a message or alike) to the R prompt. | |Note that you can always test something like this; there are several packages allowing ad-hoc compilation from `inline` to `Rcpp` and others. So here: | | > Rcpp::cppFunction(r"(void ohnoes() { assert(false); })", includes=c("#undef NDEBUG", "#include <assert.h>")) | > ohnoes() | R: file2cd1353560062b.cpp:8: void ohnoes(): Assertion `false' failed. | | Process R:3 aborted (core dumped) at Wed Dec 18 16:22:19 2024 | |... and that took my R session down just like `assert()` is expected. Also note the trickery I had to do to re-enable `assert()` by undefining `NDEBUG` and including the header after that. Exactly! You had to jump through a lot of hoops to even make assert() crash your R session. This would _not_ happen in production code. So a developer of an R extension could happily use asserts, then use your method to test his/her code. No need to remove the asserts, since NDEBUG is defined in production.
I think it would be really misleading to have code that routinely ignored the assert() calls. In a year would you remember that those asserts were effectively just comments, not being acted on without some trickery to enable them? You'd be much safer if you used a different function specific to R that triggered an R error if the assertion was false. Duncan Murdoch
| |So viewed from that angle, it really is not against _best practices for R extensions_ which is what the CRAN Policy is about. I'm not following this argument, since you (in my eyes) actually made a point for the opposite (see above). Keeping in mind that R is used mostly for scientific purposes I'd favor correctness over inconenviences like a crashing R session (keeping in mind that it would not even crash as per the arguments above). Encouraging asserts() would be a more C++-like experience for the developer, resulting in safer, more robust code, which is a good thing. Just my 50ct. cheers Chris | |Cheers, Dirk | || Therefore I would propose to remove 'assert' from the above statement. || In support, || https://cran.r-project.org/doc/manuals/r-release/R-exts.html || states that || || ``` || One usage that could call abort is the assert macro in C or C++ || functions, which should never be active in production code. The normal || way to ensure that is to define the macro NDEBUG, and R CMD INSTALL || does so as part of the compilation flags. || ``` || which seems a reasonable thing to do. || || As a sidenote, the guide could be extended to encourage new mechanisms || available in modern C++ like `static_assert`. || || || Cheers || Chris || || ______________________________________________ || R-package-devel at r-project.org mailing list || https://stat.ethz.ch/mailman/listinfo/r-package-devel | |-- |dirk.eddelbuettel.com | @eddelbuettel | edd at debian.org |
______________________________________________ R-package-devel at r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-package-devel
On 19 December 2024 at 06:06, Duncan Murdoch wrote:
| I think it would be really misleading to have code that routinely | ignored the assert() calls. In a year would you remember that those | asserts were effectively just comments, not being acted on without some | trickery to enable them? Or have a user with different compiler flags without the NDEBUG ... and now you have different behavior on different installation which is not great. | You'd be much safer if you used a different function specific to R that | triggered an R error if the assertion was false. Seconded. We trade run-time and compile-time off in other ways so this should be a run-time check, with e.g. Rf_error() or Rcpp::stop() returning to the R prompt with an error message. If you feel you must program your C++ components with assert() you could still do so in an external library effectively hiding it from CRAN check. But that's cheating. Dirk
dirk.eddelbuettel.com | @eddelbuettel | edd at debian.org
|| I think it would be really misleading to have code that routinely || ignored the assert() calls. In a year would you remember that those || asserts were effectively just comments, not being acted on without || some trickery to enable them? | |Or have a user with different compiler flags without the NDEBUG ... and now you have different behavior on different installation which is not great. There are plenty of compiler flags which change program behavior (think --ffast-math or -fno-exceptions), which the user obviously needs to accept if he uses them. Different behavior on different platforms is not necessarily a bad thing, since it sometimes catches bugs which you never would have dreamed could happen. But I think we can all agree that the behavior should be as safe as possible on all platforms. | || You'd be much safer if you used a different function specific to R || that triggered an R error if the assertion was false. | |Seconded. | That's a fair point, but know you changed the subject. The original documentation discourages assert() since it may crash the R session. Now, you are arguing that assert is not routinely enabled (due to NDEBUG) and does thus not serve any purpose. Even to the contrary it may suggest a kind of false safety when looking at code (since assert() is usually a noop). Completely different reasoning which still calls for an update of the docs and which should (repeating myself) contain a mention of static_assert. Right now, that part of the docs read as "Don't use assert, it's bad", which may translate to "Please don't write defensive/correct code" in some people's head. |We trade run-time and compile-time off in other ways so this should be a run-time check, with e.g. Rf_error() or Rcpp::stop() returning to the R prompt with an error message. If you feel you must program your C++ components with assert() you could still do so in an external library effectively hiding it from CRAN check. But that's cheating.
On 2024-12-19 7:45 a.m., Bielow, Chris wrote:
|| I think it would be really misleading to have code that routinely || ignored the assert() calls. In a year would you remember that those || asserts were effectively just comments, not being acted on without || some trickery to enable them? | |Or have a user with different compiler flags without the NDEBUG ... and now you have different behavior on different installation which is not great. There are plenty of compiler flags which change program behavior (think --ffast-math or -fno-exceptions), which the user obviously needs to accept if he uses them. Different behavior on different platforms is not necessarily a bad thing, since it sometimes catches bugs which you never would have dreamed could happen. But I think we can all agree that the behavior should be as safe as possible on all platforms. | || You'd be much safer if you used a different function specific to R || that triggered an R error if the assertion was false. | |Seconded. | That's a fair point, but know you changed the subject. The original documentation discourages assert() since it may crash the R session. Now, you are arguing that assert is not routinely enabled (due to NDEBUG) and does thus not serve any purpose. Even to the contrary it may suggest a kind of false safety when looking at code (since assert() is usually a noop). Completely different reasoning which still calls for an update of the docs and which should (repeating myself) contain a mention of static_assert. Right now, that part of the docs read as "Don't use assert, it's bad", which may translate to "Please don't write defensive/correct code" in some people's head.
I think your original message said two things: - code should use assert() - the docs about assert() need fixing. As you say, both Dirk and I were mainly addressing the first of these points. Why don't you give a suggested rewrite of the two mentions of assert() in the docs to fix them? (I'm not sure that there are only two mentions, I'm just going by the two mentions you quoted in your first message.) Duncan Murdoch
|We trade run-time and compile-time off in other ways so this should be a run-time check, with e.g. Rf_error() or Rcpp::stop() returning to the R prompt with an error message. If you feel you must program your C++ components with assert() you could still do so in an external library effectively hiding it from CRAN check. But that's cheating.