autoconf-patches
[Top][All Lists]
Advanced

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

Re: [PATCH] Rewrite handling of diversion and expansion stacks


From: Eric Blake
Subject: Re: [PATCH] Rewrite handling of diversion and expansion stacks
Date: Tue, 28 Oct 2008 23:12:06 +0000 (UTC)
User-agent: Loom/3.14 (http://gmane.org/)

Paolo Bonzini <bonzini <at> gnu.org> writes:

> 
> This patch rewrites the handling of diversion and expansion stacks,
> pushing only single entries to the stack and putting them together
> using m4_stack_foreach in the rare case a warning or error message
> needs them.

I like that concept.

> Another possible cleanup left for later is to synthesize the proper format
> of the expansion stack message in m4_expansion_stack_push, as is done
> for m4_divert_stack_push.

Yes, that needs to be done, but can come as a followup.

> 
> The new definition of m4_divert, unfortunately, introduces a small
> backwards incompatibility: all users that use the diversion stack should
> now use m4_init to push the KILL diversion; this is good practice anyway,
> and does not break Bison.  It does break a couple of tests in Autoconf's
> own testsuite, which this patch fixes.

m4_divert{_push,_pop} already required m4_init.  Only m4_divert could get away 
without it previously, but I'm okay adding that restriction.  On the other 
hand, it may be possible to push KILL at the bottom of the diversion stack at 
the end of m4sugar.m4 outside of m4_init; it should still work across frozen 
files, and would make m4_divert work out-of-the-box.  I'll play with that 
idea.  But your changes to the testsuite are acceptable, whether or not I get a 
subsequent patch that lets m4_divert work in isolation once again.

> 
> Okay?

A couple of nits, but ready to apply.

> 
> +** m4sugar requires m4_init in order to use m4_divert, m4_divert_push
> +   and m4_divert_pop.

Only m4_divert changed; m4_divert_push and m4_divert_pop already required 
m4_init.

> -# The scheme is simplistic: each time we enter an m4_defun'd macros,
> -# we prepend its name in m4_expansion_stack, and when we exit the
> -# macro, we remove it (thanks to pushdef/popdef).
> +# Each time we enter an m4_defun'd macros, we add a definition in
> +# m4_expansion_stack, and when we exit the macro, we remove it (thanks

s/m4_expansion_stack/_&/

>  
> -# m4_newline
> -# ----------
> -# Expands to a newline.  Exists for formatting reasons.
> +# m4_newline(STRING)

Since STRING is optional, I'd format this:
# m4_newline([STRING])

> +# ------------------
> +# Expands to a newline, possibly followed by STRING.  Exists mostly for
> +# formatting reasons.

[m4_newline[]STRING], [m4_newline([STRING])], and even [
STRING] are all identical, with the last being the most efficient.  Oh, I see - 
making m4_newline take an argument makes it useful for m4_stack_foreach_lifo.  
Cute.  You could have also used [m4_newline[]m4_echo] in that context, but that 
is twice as many macro expansions.  With my pending patch for 
m4_stack_foreach_sep, this could be done without any extra macros (not that 
diversion/expansion stack dumping is on the hot path to be worrying about extra 
macros):

m4_stack_foreach_sep_lifo([_m4_expansion_stack], [], [
])

I also like how the flexibility of always prepending, always appending, or only 
using newline as a separator between entries, is better exposed with 
m4_stack_foreach_sep*.  But I can fold that in later when I rebase on top of 
you and when we standardize the formatting between the two stacks; the 
m4_newline change seems fine as is for this commit.

-- 
Eric Blake






reply via email to

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