Check -W -Wall -ansi

[wryun]

Just because we can.

[wryun]

Turned on some (fdf29d5), disabled others so it still compiles without noise. Should probably check the clobbering warning.

[jpco]

While I think it makes sense for es to be built in "native mode" for practical use, I also think that for testing/development purposes, it makes sense to add a strict target to the Makefile that builds es with strict standards-compliance flags. Specifically, I'm picturing adding -ansi -D_POSIX_C_SOURCE=200112L to enforce ANSI C89 and POSIX.1-2001, which seem to be the current standards being targeted.

Current blockers to successfully building with those two flags defined:

• Some way to get at a correct NSIG-equivalent value. Or perhaps just using a hardcoded signal list when building in strict mode, based directly on what POSIX specifies (really, it covers almost every signal es cares about).
• Fix the va_copy call in string.c:279 (va_copy isn't present in ANSI C89).

We could even add -Werror -pedantic if we wanted to be really strict. Blockers to having that working, in addition to the previous, are:

• conversions between function and object pointers.
• "ISO C forbids conditional expr with only one void side" caused by the QX macro in match.c.

This strict-mode es could be used in the CircleCI tests to help prevent easy-to-miss bugs/unportabilities slipping in.

[memreflect]

While I think it makes sense for es to be built in "native mode" for practical use, I also think that for testing/development purposes, it makes sense to add a strict target to the Makefile that builds es with strict standards-compliance flags. Specifically, I'm picturing adding -ansi -D_POSIX_C_SOURCE=200112L to enforce ANSI C89 and POSIX.1-2001, which seem to be the current standards being targeted.

That's reasonable, and it definitely would help catch some simple standards-compliance issues.
For example, when compiling with GCINFO, GCVERBOSE, and LISPTREES enabled, usage() in main.c contains a string literal that is 613 characters in length after concatenation, but C90 compilers are only required to support string literals up to 509 characters.

Current blockers to successfully building with those two flags defined:

• Some way to get at a correct NSIG-equivalent value. Or perhaps just using a hardcoded signal list when building in strict mode, based directly on what POSIX specifies (really, it covers almost every signal es cares about).

Perhaps mksignal should only add signals from a list of known signals?
It's a more conservative approach, but it means things like SIGSTKSZ don't need to be ignored, and the #if SIGxxx < NSIG check can be eliminated.

Similarly, the nsignals variable in sigmsgs.h could be replaced with an NSIGNALS macro that serves the same role as NSIG, with the awk script emitting an integer constant instead of arraysize(signals) by only counting unique signals (i.e. those that are defined as integers rather than as aliases for other signals).

• Fix the va_copy call in string.c:279 print.c:279 (va_copy isn't present in ANSI C89).

Storing a va_list * inside Format eliminates all dependencies on va_copy() that i have seen; assigning pointers of compatible types is always well-defined.

Doing that also eliminates an issue with C's "undefined behavior".
In the case of ANSI C89 and newer, va_start() must be matched by a corresponding va_end() within the same function, which i think es does correctly.
C99 and later, and POSIX.1-2001 and later, define va_copy() with the same requirement, however, which creates a problem because that exact line you mentioned doesn't fulfill that requirement.
va_copy() might allocate memory that needs to be freed with va_end() for example.

There's also the fact that glibc doesn't define va_copy() with -D_POSIX_C_SOURCE=200112L because it leaves it up to the compiler.
Worse, gcc only defines va_copy() when compiling in C99 mode or newer.
It does provide __va_copy(), however, so if es needs a va_copy() in C89 mode someday, configure tests can help.

• "ISO C forbids conditional expr with only one void side" caused by the QX macro in match.c.

That's definitely my oopsie.
When i wrote the QX macro, i was thinking of a standalone statement 0;, which needs to be written (void)0; to silence compiler diagnostics about it not being useful.

• conversions between function and object pointers.

This is required by POSIX.1-2008, but not by POSIX.1-2001 unless the XSI option is supported; see the Rationale for POSIX.1-2001 dlsym().

If this is a problem, maybe run the preprocessor on all C files to generate an array of primitives since the number of primitives is relatively small (currently maxed out 54).
Once all primitive names have been added, the array could be sorted with qsort() by primitive name in initprims(), so finding a primitive using bsearch() or another binary search algorithm would still be fast.
Then the entire API surrounding primitives would be adjusted to use the array instead of a Dict.

Sample implementation (includes a fixed QX() macro and fixed usage())

[jpco]

If this is a problem, maybe run the preprocessor on all C files to generate an array of primitives since the number of primitives is relatively small (currently maxed out 54).
Once all primitive names have been added, the array could be sorted with qsort() by primitive name in initprims(), so finding a primitive using bsearch() or another binary search algorithm would still be fast.
Then the entire API surrounding primitives would be adjusted to use the array instead of a Dict.

Sample implementation (includes a fixed QX() macro and fixed usage())

Wow, that's sophisticated. I was just thinking of wrapping the primitive functions inside a small struct. :)

#include <stdio.h>

int prim_func(int i) { return i + 1; }

int lookatthevoid(void *p) { printf("%d\n", sizeof(p)); }

int main(int c, char **argv) {
	struct { int (*prim)(int); } p;

	/* not allowed with -ansi -pedantic */
	lookatthevoid((void *)&prim_func);

	p.prim = prim_func;
	/* allowed with -ansi -pedantic */
	lookatthevoid((void *)&p);
}

[jpco]

Here's my quick-and-dirty diff for -ansi -pedantic-compatible primitives:

diff --git a/prim.c b/prim.c
index 13a66ae..74d7866 100644
--- a/prim.c
+++ b/prim.c
@@ -6,11 +6,11 @@
 static Dict *prims;
 
 extern List *prim(char *s, List *list, Binding *binding, int evalflags) {
-	List *(*p)(List *, Binding *, int);
-	p = (List *(*)(List *, Binding *, int)) dictget(prims, s);
+	Prim *p;
+	p = (Prim *) dictget(prims, s);
 	if (p == NULL)
 		fail("es:prim", "unknown primitive: %s", s);
-	return (*p)(list, binding, evalflags);
+	return (p->prim)(list, binding, evalflags);
 }
 
 static char *list_prefix;
diff --git a/prim.h b/prim.h
index 68e346b..9c2f249 100644
--- a/prim.h
+++ b/prim.h
@@ -1,13 +1,19 @@
 /* prim.h -- definitions for es primitives ($Revision: 1.1.1.1 $) */
 
+typedef struct { List *(*prim)(List *, Binding *, int); } Prim;
+
 #define	PRIM(name)	static List *CONCAT(prim_,name)( \
 				List UNUSED *list, Binding UNUSED *binding, int UNUSED evalflags \
 			)
-#define	X(name)		(primdict = dictput( \
-				primdict, \
-				STRING(name), \
-				(void *) CONCAT(prim_,name) \
-			))
+
+#define	X(name)		STMT( \
+				static Prim CONCAT(prim_struct_,name) = { CONCAT(prim_,name) }; \
+				primdict = dictput( \
+					primdict, \
+					STRING(name), \
+					(void *) &CONCAT(prim_struct_,name) \
+				) \
+			)
 
 extern Dict *initprims_controlflow(Dict *primdict);	/* prim-ctl.c */
 extern Dict *initprims_io(Dict *primdict);		/* prim-io.c */

[memreflect]

Wow, that's sophisticated. I was just thinking of wrapping the primitive functions inside a small struct. :)

That...makes more sense and is a lot simpler to maintain.
Perhaps one day i will be able to drop my habit of overengineering solutions. :)