[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: more M4sh documentation

From: Paul Eggert
Subject: Re: more M4sh documentation
Date: Wed, 22 Mar 2006 13:30:45 -0800
User-agent: Gnus/5.1007 (Gnus v5.10.7) Emacs/21.4 (gnu/linux)

Ralf Wildenhues <address@hidden> writes:

> the real question is one of commitment to these interfaces.

I think that's reasonable (especially if you're willing to help
maintain them :-).  But please see below first; I've done a more
careful review this time, and we need at least one more go-round.

> Other issues fixed in this version: do not use TABs in .texi files
> (could that be checked, say, in Makefile.maint?)

I don't see why not.

> address@hidden AS_EXECUTABLE_P (@var{file-name})
> address@hidden
> +Check whether @file{file-name} is executable (@pxref{Limitations of
> +Builtins}).

Let's not document this now, since AS_EXECUTABLE_P also checks whether
the file is a regular file, and it doesn't check whether it's
executable on some platforms.  We need a better name for this macro,
or a better implementation, or both.

> address@hidden AS_LITERAL_IF (@var{expression}, @var{if-literal}, 
> @var{if-not-literal})
> address@hidden
> +If @var{expression} contains shell indirections such as parameter
> +expansion @samp{$var} or command substitution @samp{`command`}, expand
> +to @var{if-not-literal}, else expand to @var{if-literal}.

First, this is backwards (the if- and the if-not).  Second, it doesn't explain
things precisely.  Better would be:

   If @var{expression} might contain shell indirections such as parameter
   expansion @samp{$var} or command substitution @samp{`command`}, expand
   to @var{if-literal}.  If it cannot possibly contain these constructs,
   expand to @var{if-literal}.

> The macro
> +only approximates the answer, as some literals will not be recognized as
> +such.

I didn't quite follow this sentence, but I expect that you can remove
it now, assuming you like the previous suggestion.

> It may be used to create macros that are polymorphic to M4 and
> +shell arguments, or to cope with variables both without and with
> +indirection, such as @samp{ac_cv_$var}.

Sorry, I didn't quite follow this.  How about two examples, one for
each scenario?

> address@hidden AS_LN_S (@var{file}, @var{link})
> address@hidden
> +Link @file{file} to @file{link} using @samp{ln -s} if the operating
> +system and file system support symbolic links, otherwise using @samp{ln}
> +if that works, otherwise using @samp{cp -p}.
> address@hidden defmac

This documentation is incorrect, since AS_LN_S makes the decision just
once, at the start of execution.  For example, if some file systems
support symbolic links but others don't, AS_LN_S will screw up.  Here
I prefer the documentation to the actual behavior; can you please fix
the behavior?  Until then, I'd rather leave it undocumented.

> address@hidden AS_SET_CATFILE (@var{var}, @var{dir}, @var{file})
> address@hidden
> +Set the shell variable @var{var} to the location of path name
> address@hidden from the perspective of directory @var{dir}, optimizing
> +for common cases (@var{dir} or @var{file} is @samp{.}, @var{file}
> +is absolute).  This macro expects portable path names (@pxref{File
> +System Conventions}).

First, use "file name", not "path name", as per the GNU style
conventions.  Second, how about if you remove the last sentence?  I
don't see why it's needed.

> address@hidden AS_TMPDIR (@var{prefix}, @dvar{directory, $TMPDIR})
> address@hidden
> +Create a temporary directory in @var{directory}, as safely as possible
> +(@pxref{Limitations of Usual Tools}).
> address@hidden defaults to @env{TMPDIR}, which is defaulted to
> address@hidden/tmp}.  The @var{prefix} should be up to 4 characters long and

up to 4 characters -> 1 to 4 bytes

> +indicate the script responsible for the directory.  If successful,

indicate -> should indicate

> +set @env{tmp} to the name of the newly created directory, otherwise

Is it really wise to always set a global variable like that?  Perhaps
the name of the variable should be an additional, optional argument to

> +terminate the script.  No provision for the removal of @samp{$tmp} is
> +made.

Please add something like this:

   In the current implementation, there is a race condition between
   the time the directory is created and the time that @samp{$tmp} is
   assigned to.  If a signal arrives during that time, @samp{$tmp}
   will have its old value even though the directory has been created.
   This race condition is a bug, and the interface to @code{AS_TMPDIR}
   may be changed in the future to work around it.

Also, please fix the code to have the behavior described above, before
we release the documentation this way.  (Currently the code has this
behavior only on hosts where mktemp works.)

> +With the help of @samp{AS_LITERAL_IF}, it is possible to unify the
> +handling of M4 and shell variables together with indirections to some
> +extent.

Sorry, I don't understand this wording.  What exactly is the problem?
How about an example of the problem and why a solution is needed,
before diving straight into the solution?  For example:

> address@hidden AS_VAR_GET (@var{variable})
> address@hidden
> +Get the value of shell variable @var{variable}.
> address@hidden defmac

Why not just use "$variable"?

and so forth for the other macros.

> +A possible implementation of @samp{AC_CHECK_FUNC} demonstrates the use
> +of these accessor macros.  Note that it allows both literals and shell
> +variables in the first argument:
> +
> address@hidden
> +[AS_VAR_PUSHDEF([ac_var], [ac_cv_func_$1])dnl
> +AC_CACHE_CHECK([for $1], ac_var,
> +                [AS_VAR_SET(ac_var, yes)],
> +                [AS_VAR_SET(ac_var, no)])])
> +AS_IF([test AS_VAR_GET(ac_var) = yes], [$2], [$3])dnl
> +AS_VAR_POPDEF([ac_var])dnl
> +])
> address@hidden example

This example is fine as far as it goes, but it doesn't really show why
the macros are necessary or useful.  Perhap you could contrast it with
an attempt to implement AC_CHECK_FUNC without using these macros.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]