Re: [PATCH 1/4 v1.1] Remove Wundef warnings for specification macros

[Roland McGrath <roland at hack dot frob dot com>]

"confdefs-defs" just seems like an inane name.  Please find something better.

I think "posix-conf-vars.list" would be a better name than "conf.list",
which sounds too generic.

> +$(objpfx)confdefs-defs.h: conf.list Makefile $(..)scripts/gen-conf.awk
> +	$(make-target-directory)
> +	$(AWK) -f $(..)scripts/gen-conf.awk $< > $@.tmp
> +	mv -f $@.tmp $@

The usual way we do this is:

$(objpfx)confdefs-defs.h: $(..)scripts/gen-conf.awk conf.list Makefile 
[...]
	$(AWK) -f $(filter-out Makefile, $^) > $@.tmp

> --- /dev/null
> +++ b/posix/confdefs.h
> @@ -0,0 +1,15 @@
> +#ifndef __CONFDEFS_H__
> +#define __CONFDEFS_H__

Missing comment and copyright header.  Guard macro is just _FOO_H, no
extra leading underscore and no trailing underscores.

> +#include <posix/confdefs-defs.h>

Where does posix/confdefs-defs.h come from??  Is that just hitting because
of -I$(common-objdir) and the generated code happens to be in the posix/
build subdirectory?

If you're including the generated file, just <confdefs-defs.h> is the right
syntax.  If it needs to be used by code compiled outside the posix/
subdirectory (which I don't think it should), then put the generated file
in $(common-objdir) directly.

As well as a new name for both this file and the generated file, this file
needs substantial commentary explaining the rationale and how to use the
macros.

> +#define CONF_DEF_UNDEFINED 1
> +#define CONF_DEF_DEFINED_SET 2
> +#define CONF_DEF_DEFINED_UNSET 3
> +
> +#define CONF_IS_DEFINED_SET(conf) (conf##_DEF == CONF_DEF_DEFINED_SET)
> +#define CONF_IS_DEFINED_UNSET(conf) (conf##_DEF == CONF_DEF_DEFINED_UNSET)
> +#define CONF_IS_UNDEFINED(conf) (conf##_DEF == CONF_DEF_UNDEFINED)
> +#define CONF_IS_DEFINED(conf) (conf##_DEF != CONF_DEF_UNDEFINED)

Style is to use tabs between the lhs and rhs to line up all the rhs.

Incidentally, it seems more proper to me to use a prefix for the
constructed magic macro names, rather than a suffix.  
i.e. CONF_DEF_##conf or the like.

> --- a/posix/confstr.c
> +++ b/posix/confstr.c
> @@ -21,6 +21,7 @@
>  #include <string.h>
>  #include <confstr.h>
>  #include "../version.h"
> +#include "confdefs.h"

Use <> syntax for generated files (or sysdeps files, or almost any case in
libc).

> --- /dev/null
> +++ b/scripts/gen-conf.awk

Let's give it a less generic-sounding name.  Maybe "posix-conf-vars"?

> +  PROCINFO["sorted_in"] = "@val_type_asc"

I couldn't even find what this means in the Gawk manual.  It's probably
better to avoid such obscure features if it's not prohibitive.  At the very
least, you need a clear comment about what this magic does.

> +# Begin a new prefix.
> +$2 == "{" {
> +  split ($1, arr, ":")
> +  type = arr[1]
> +  prefix = arr[2]
> +  next
> +}

Why not just make the syntax use separate words rather than splitting on a
colon?  If some are optional, match with $NF == "{".

> +{
> +  if (prefix == "" && type == "" && sc_prefix == "") {
> +    print "Syntax error" > "/dev/stderr"
> +    exit 1
> +  }

You can use FILENAME and FNR to produce C-x `-compatible error messages.

> +ENDFILE {

Just use END.

> +  print "/* Autogenerated by gen-conf.awk.  */\n"

Include "DO NOT EDIT!".

This is OK with those cleanups.


Thanks,
Roland