Re: [Lightning] config.h should not be included in the library header

[Marc Nieper-Wißkirchen]

Dear Paulo,

here is a patch. The actual code seen by the compiler didn't change so
it is safe even for some ancient targets:

--- PATCH BEGIN ---

diff --git a/ChangeLog b/ChangeLog
index 150af31..3b445a1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2019-08-29 Marc Nieper-Wißkirchen <address@hidden>
+
+    * include/lightning/jit_private.h: Move definition of offsetof
+    from the public header file here.
+
+    * configure.ac, include/Makefile.am, include/lightning.h,
+    include/lightning.h.in: Generate lightning.h from lightning.in.h
+    and remove the dependence on config.h from the public header file.
+
 2019-06-04 Paulo Andrade <address@hidden>

     * include/lightning/jit_riscv.h, lib/jit_riscv-cpu.c,
diff --git a/configure.ac b/configure.ac
index f973b53..ba28ce4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -285,10 +285,15 @@ AC_SUBST(cpu)

 AC_SUBST([LIGHTNING_CFLAGS])

+if test $ac_cv_header_stdint_h = yes; then
+    AC_SUBST(MAYBE_INCLUDE_STDINT_H, ["#include <stdint.h>"])
+fi
+
 AC_OUTPUT([Makefile
        lightning.pc
        doc/Makefile
-       include/Makefile
+           include/Makefile
        include/lightning/Makefile
-       lib/Makefile
+       include/lightning.h
+           lib/Makefile
        check/Makefile])
diff --git a/doc/version.texi b/doc/version.texi
index 1d47234..44b229d 100644
--- a/doc/version.texi
+++ b/doc/version.texi
@@ -1,4 +1,4 @@
-@set UPDATED 4 September 2017
-@set UPDATED-MONTH September 2017
-@set EDITION 2.1.1
-@set VERSION 2.1.1
+@set UPDATED 29 August 2019
+@set UPDATED-MONTH August 2019
+@set EDITION 2.1.2
+@set VERSION 2.1.2
diff --git a/include/Makefile.am b/include/Makefile.am
index 601ae7a..7914e31 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -17,3 +17,5 @@
 include_HEADERS = lightning.h
 SUBDIRS =        \
     lightning
+
+nodist_include_HEADERS = lightning.h
diff --git a/include/lightning.h b/include/lightning.h.in
similarity index 99%
rename from include/lightning.h
rename to include/lightning.h.in
index 8a142a2..9030fa8 100644
--- a/include/lightning.h
+++ b/include/lightning.h.in
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2017  Free Software Foundation, Inc.
+ * Copyright (C) 2012-2017, 2019  Free Software Foundation, Inc.
  *
  * This file is part of GNU lightning.
  *
@@ -20,15 +20,9 @@
 #ifndef _lightning_h
 #define _lightning_h

-#if HAVE_CONFIG_H
-# include "config.h"
-#endif
-
 #include <unistd.h>
 #include <stdlib.h>
-#if HAVE_STDINT_H
-#  include <stdint.h>
-#endif
+@MAYBE_INCLUDE_STDINT_H@
 #include <string.h>

 #if defined(__hpux) && defined(__hppa__)
@@ -38,14 +32,6 @@
 #  include <machine/endian.h>
 #endif

-#ifdef STDC_HEADERS
-#  include <stddef.h>
-#else
-#  if !defined(offsetof)
-#    define offsetof(type, field) ((char *)&((type *)0)->field - (char *)0)
-#  endif
-#endif
-
 #ifndef __WORDSIZE
 #  if defined(WORDSIZE)                /* ppc darwin */
 #    define __WORDSIZE        WORDSIZE
diff --git a/include/lightning/jit_private.h b/include/lightning/jit_private.h
index 9e0a8be..492d070 100644
--- a/include/lightning/jit_private.h
+++ b/include/lightning/jit_private.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2017  Free Software Foundation, Inc.
+ * Copyright (C) 2012-2017, 2019  Free Software Foundation, Inc.
  *
  * This file is part of GNU lightning.
  *
@@ -28,6 +28,14 @@
 #include <limits.h>
 #include <stdio.h>

+#ifdef STDC_HEADERS
+#  include <stddef.h>
+#else
+#  if !defined(offsetof)
+#    define offsetof(type, field) ((char *)&((type *)0)->field - (char *)0)
+#  endif
+#endif
+
 #if defined(__GNUC__)
 #  define maybe_unused        __attribute__ ((unused))
 #  define unlikely(exprn)    __builtin_expect(!!(exprn), 0)

--- PATCH END ---

What I haven't changed yet (but should be considered to be changed in
some future version of lightning) is renaming of "__WORDSIZE",
"__LITTLE_ENDIAN", and so forth, to something like "JIT_WORDSIZE",
"JIT_LITTLE_ENDIAN" so that we don't mess with reserved namespaces.
(There may be more "__"-identifiers occurring in the machine-specific
include files.

Best,

Marc

Am Mi., 28. Aug. 2019 um 20:42 Uhr schrieb Paulo César Pereira de
Andrade <address@hidden>:
>
> Em dom, 25 de ago de 2019 às 04:30, Marc Nieper-Wißkirchen
> <address@hidden> escreveu:
> >
> > Dear Paulo,
>
>   Hi, Sorry for the delay responding,
>
> > the first lines of <lightning.h> currently read:
> >
> > **
> > #ifndef _lightning_h
> > #define _lightning_h
> >
> > #if HAVE_CONFIG_H
> > # include "config.h"
> > #endif
> >
> > #include <unistd.h>
> > #include <stdlib.h>
> > #if HAVE_STDINT_H
> > #  include <stdint.h>
> > #endif
> > #include <string.h>
> >
> > #if defined(__hpux) && defined(__hppa__)
> > #  include <machine/param.h>
> > #endif
> > #if defined(__alpha__) && defined(__osf__)
> > #  include <machine/endian.h>
> > #endif
> >
> > #ifdef STDC_HEADERS
> > #  include <stddef.h>
> > #else
> > #  if !defined(offsetof)
> > #    define offsetof(type, field) ((char *)&((type *)0)->field - (char *)0)
> > #  endif
> > #endif
> > **
> >
> > I would like to suggest to remedy the following three things:
> >
> > 1) The header file should not include "config.h" (and test
> > HAVE_CONFIG_H) because it would include the "config.h" (and test the
> > preprocessor flag) of the application using GNU lightning and not the
> > "config.h" and the preprocessor flag of GNU lightning.
> >
> > 2) Likewise, the test about HAVE_STDINT_H is meaningless as long as
> > the program using GNU lightning does not set the appropriate
> > preprocessor flag.
>
>   Agreed. Only include/jit_private.h should check for config.h.
>
> > 3) A general header file should not (re-)define "offsetof". As far as
> > I see, it is not needed in the include file anyway.
> >
> > In other words, 3) is easily corrected by moving the definition of
> > "offsetof" into a header file that is private to the lightning
> > sources.
>
>   Yes, it is trivial to move the definition to jit_private.h.
>
> > 1) and 2) can be corrected by replacing "lightning.h" by
> > "lightning.h.in" and using autoconf substitutions to generate a
> > customized "lightning.h". In the same go, one can also rewrite the
> > WORDSIZE tests as autoconf tests. This has also the more than welcome
> > side effect that "__WORDSIZE" (which should be considered private to
> > the compiler and standard library) doesn't need to be touched (and
> > redefined) by <lightning.h>.
>
>   I was thinking of just adding some autoconf macros to -D__WORDSIZE=
> as appropriate, but it would not work because it needs to know the word size
> for a few macros and prototypes.
>
>   Would you mind in making the patch for lightning.h.in? Otherwise I can
> write one in the next few days, and hopefully soon make a new release,
> including the new riscv port. If you do the patch, it would be better if done
> in a safe way, to not break a few test platforms I no longer have access,
> like HP-UX, ppc Darwin (this should not be too hard to get a vm),  IRIX,
> and Tru64.
>
> > Thanks!
> >
> > Marc
>
> Thanks!
> Paulo