Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New CRAN Issue: boolean.h creating errors on r-devel Debian gcc #6779

Closed
TysonStanley opened this issue Jan 30, 2025 · 10 comments · Fixed by #6782
Closed

New CRAN Issue: boolean.h creating errors on r-devel Debian gcc #6779

TysonStanley opened this issue Jan 30, 2025 · 10 comments · Fixed by #6782
Labels
Milestone

Comments

@TysonStanley
Copy link
Member

Just got this message from CRAN and co:

That seems to be from the change to src/include/R_ext/Boolean.h of

r87656 | ripley | 2025-01-28 13:27:50 +0100 (Tue, 28 Jan 2025) | 1 line
add section on C23doc/manual/R-exts.texi

Please investigate.

Error is on r-devel for Debian gcc https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-debian-gcc/data.table-00check.html tied to the change shown for Jan 29 https://developer.r-project.org/blosxom.cgi/R-devel/NEWS.

What I'm seeing as the diff is:

Index: src/include/R_ext/Boolean.h
===================================================================
--- src/include/R_ext/Boolean.h	(revision 87655)
+++ src/include/R_ext/Boolean.h	(revision 87656)
@@ -1,6 +1,6 @@
 /*
  *  R : A Computer Language for Statistical Data Analysis
- *  Copyright (C) 2000, 2001 The R Core Team.
+ *  Copyright (C) 2000, 2025 The R Core Team.
  *
  *  This header file is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU Lesser General Public License as published by
@@ -29,13 +29,56 @@
 #undef FALSE
 #undef TRUE
 
+/* This can eventually be simplified to
+#if (!defined __cplusplus) && !(defined __STDC_VERSION__ && __STDC_VERSION__ > 201710L)
+# include <stdbool.h>
+#endif
+
 #ifdef  __cplusplus
 extern "C" {
+typedef bool Rboolean;
+#define FALSE false
+#define TRUE true
+}
+#define _R_RBOOLEAN_IS_BOOL_ 1
 #endif
-typedef enum { FALSE = 0, TRUE /*, MAYBE */ } Rboolean;
+*/
 
 #ifdef  __cplusplus
+
+extern "C" {
+    /* once cp11 is soerted
+typedef bool Rboolean;
+#define FALSE false
+#define TRUE true
+    */
+typedef enum { FALSE = 0, TRUE } Rboolean;
 }
+
+# else
+
+// Also include standard C boolean type
+#if defined __STDC_VERSION__ && __STDC_VERSION__ > 201710L
+// C23 so these are keywords
+#else
+// stdbool.h is C99, so available everywhere
+//# include <stdbool.h>
 #endif
 
+/* 
+   __bool_true_false_are_defined is defined in stdbool.h, and C23, but
+   it and stdbool.h are declared obsolescent in C23.
+*/
+#ifdef __bool_true_false_are_defined
+typedef bool Rboolean;
+# define FALSE false
+# define TRUE true
+# define _R_RBOOLEAN_IS_BOOL_ 1
+#else
+typedef enum { FALSE = 0, TRUE } Rboolean;
+#endif //__bool_true_false_are_defined
+
+# endif // __cplusplus
+
+
 #endif /* R_EXT_BOOLEAN_H_ */
Index: doc/NEWS.Rd
===================================================================
--- doc/NEWS.Rd	(revision 87655)
+++ doc/NEWS.Rd	(revision 87656)
@@ -281,6 +281,20 @@
       \item The deprecated and seemingly never-used S-compatibility
       macros \code{F77_COM} and \code{F77_COMDECL} have been removed
       from header \file{R_ext/RS.h}.
+
+      \item Header \file{R_ext/Rboolean.h} now defines the type
+      \code{Rboolean} as an alias of the standard C type \code{bool}.
+
+      Currently this is done from C (not C++) only if
+      \code{__bool_true_false_are_defined} is defined, which it is in
+      C23 mode or if the C99 header \file{stdbool.h} is included.
+      
+      Before release \file{stdbool.h} will be included from C if C23 mode is
+      not supported, and C++ \code{bool} will be used if called from
+      C++.
+
+      The macro \code{_R_RBOOLEAN_IS_BOOL_} is defined where
+      \code{Rboolean} is implemented \emph{via} \code{bool}.
     }
   }
 
@@ -603,10 +617,6 @@
       \item \code{as.roman(x)} now should work platform independently,
       also for, e.g., \code{x = "IIIII"} (= V) and \code{x = "IIIIII"}
       (= VI).
-
-      \item \command{R CMD Rd2pdf} works again on an installed package
-      directory containing \LaTeX help (from option \option{--latex}),
-      thanks to a report by \I{Peter Ruckdeschel}.
     }
   }
 }
Index: doc/manual/R-exts.texi
===================================================================
--- doc/manual/R-exts.texi	(revision 87655)
+++ doc/manual/R-exts.texi	(revision 87656)
@@ -6569,6 +6569,51 @@
 @uref{https://github.com/mstorsjo/llvm-mingw}: this uses @code{libc++}.
 
 
+@node C23 changes
+@subsubsection C23 changes
+
+Thw C23 standard was finally published in Oct 2024, by which time had
+been widely implemented for a least a couple of years.  It will become
+the default on GCC 15, and @R{} will use default to it if available from @R{}
+4.5.0.
+
+Some of the more significant changes are
+@c https://en.cppreference.com/w/c/23
+
+@itemize
+
+@item
+@code{bool}, @code{true} and @code{false} become keywords and so can no
+longer be used as identifiers.
+
+These have been available as a boolean type since C99 by including the
+header @file{stdbool.h}.  Both that and C23@footnote{but C23 declares
+that header and the macro to be obsolescent.} set the macro
+@code{__bool_true_false_are_defined} to @code{1} so this type can be
+used in all versions of C supported by @R{}.
+
+@item
+The meaning of an empty argument list has been changed to mean zero
+arguments --- however for clarity @code{fun(void)} is still preferred by
+many code readers and supported by all C standards.  (Compilers may warn
+about an empty argument list in C23 mode.)
+
+@item
+@code{INIINITY} and @code{NAN} are available via
+header @file{float.h} and deprecated in @file{math.h}.
+
+@item
+POSIC functions @code{memccpy}, @code{strdup} and @code{strndup} are
+part of C23.
+
+@item
+There are decimal floating-point types and functions and extended
+support of binary floating-point functions, including binary
+floating-point constants.
+@end itemize
+
+
+
 @node Portable Fortran code
 @subsection Portable Fortran code
@MichaelChirico
Copy link
Member

Thanks to Brian pinning down the responsible change in r-devel. I've been having trouble reproducing it but it seems something we need to handle anyway.

Did Brian give a date for a required update?

Probably the change should just be shipped with 1.17.0 at this point, as opposed to a new patch.

@MichaelChirico MichaelChirico added this to the 1.17.0 milestone Jan 30, 2025
@TysonStanley TysonStanley changed the title New CRAN Issue: New CRAN Issue: boolean.h creating errors on r-devel Debian gcc Jan 30, 2025
@TysonStanley
Copy link
Member Author

This was a message from Kurt but agree, pointing to the commit was really nice.

Forgot to include that, he said: 2025-02-20

@MichaelChirico
Copy link
Member

Perma-linking the mirrored version of the change:

r-devel/r-svn@984655d

@MichaelChirico
Copy link
Member

Jan's dockerfiles repo is currently failing its pipeline (cc @jangorecki + @ben-schwen), so the codespaces are also using a too-old version of r-devel:

https://gitlab.com/jangorecki/dockerfiles/-/pipelines?page=2&scope=all

@MichaelChirico
Copy link
Member

Looks like there's simply no more free CI minutes to use:

Image

Is this something can be resolved with the recent free minutes for OSS projects Ben worked out?

@ben-schwen
Copy link
Member

Looks like there's simply no more free CI minutes to use:

Image

Is this something can be resolved with the recent free minutes for OSS projects Ben worked out?

Yes if we move the dockerfiles to the data.table group repo. There we have 50k minutes per month IINM

@aitap
Copy link
Contributor

aitap commented Jan 30, 2025

What r87656 broke is the ability to assume that LGLSXP vectors contain Rbooleans and maybe have them store NA_LOGICAL. (Previously Rboolean was an enum, which in practice meant a fancy int.) Now that Rboolean is an alias for C bool, it can only store 0 or 1, nothing else, and most likely differs in size from int.

Meanwhile, LOGICAL() returns an int*, so now we cannot safely cast it to Rboolean*. This seems to be a one-line fix, actually!

@MichaelChirico
Copy link
Member

Thanks! I think I've been fundamentally not understanding the Rboolean type, described in WRE:

Further, the included header R_ext/Boolean.h has enumeration constants TRUE and FALSE of type Rboolean in order to provide a way of using “logical” variables in C consistently. This can conflict with other software: for example it conflicts with the headers in IJG’s jpeg-9 (but not earlier versions).

I had been thinking Rboolean is the "correct" type for LOGICAL(), i.e. that when we treat LOGICAL() as int*, we're exposing ourselves to R eventually changing storage type for LGLSXP. But i guess it's actually just a long-time workaround for the lack of "true" fundamental boolean types in C?

@aitap
Copy link
Contributor

aitap commented Jan 30, 2025

Yes, stdbool.h only appeared in C99, and everyone invented their own booleans in the preceding decades. I too thought that Rbooleans were deliberately defined in a type that was in practice compatible with LOGICAL(...) buffers. It's one of the things that would be great to see clarified in API documentation.

Post-factum it's easier to notice that LOGICAL_OR_NULL is explicitly documented to return an int*, or that any time an R API function takes an Rboolean argument, it doesn't test for NA_LOGICAL.

@MichaelChirico
Copy link
Member

Raised on r-devel: https://stat.ethz.ch/pipermail/r-devel/2025-January/083813.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants