Re: [groff] 02/07: tmac/an-old.tmac: Refactor DT and PD.

[Ingo Schwarze]

Hi Branden,

G. Branden Robinson wrote on Thu, May 20, 2021 at 01:23:16AM -0400:

> commit 69863462003ed26b8da81ab174b0a6c0a7d26eea
> Author: G. Branden Robinson <g.branden.robinson@gmail.com>
> AuthorDate: Thu May 20 10:28:26 2021 +1000
> 
>     tmac/an-old.tmac: Refactor DT and PD.
>     
>     * tmac/an-old.tmac: Refactor to move bodies of DT and PD into private
>       macros.
>     
>       (an-reset-tab-stops, an-reset-paragraphp-spacing): New names for the
>       former DT and PT.
>     
>       (TH): Call these new macro names.
>     
>       (DT, PD): Wrap the corresponding private macros.
> ---
>  ChangeLog        |  9 +++++++++
>  tmac/an-old.tmac | 18 +++++++++++++-----
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
[...]
> diff --git a/tmac/an-old.tmac b/tmac/an-old.tmac
> index 8f28c7a..40d4403 100644
> --- a/tmac/an-old.tmac
> +++ b/tmac/an-old.tmac
[...]
> +.de an-reset-paragraph-spacing
> +.  ie \\n[.$] .nr PD (v;\\$1)
> +.  el         .nr PD (.4v >? \n[.V])
> +..
[...]
>  .de1 PD
> -.  ie \\n[.$] .nr PD (v;\\$1)
> -.  el         .nr PD (.4v >? \n[.V])
> +\\*[an-reset-paragraph-spacing]\\
>  ..

I think this is terrible roff(7) coding style and should be reverted.

I needed to scratch my head for a considerable time to even understand
why and how it works, even though i'm maintaining a roff(7) parser
myself.  How are people supposed to understand such obfuscated code
who do not maintain roff parsers?

It appears your code works because your new implementation of the
.PD macro delays evaluation of the \*[an-reset-paragraph-spacing]
string until macro expansion time.  At expansion time, the
.ie-.el-roff(7) code gets inserted into the input stream, expanding
the \\$1 argument, and since the inserted code happens to be at the
beginning of an input line, the inserted code gets then parsed as
roff(7) input, setting the .PD register as desired.  It would no
longer work when indented, so it's fragile in addition to being
visually misleading.  By the way, your implementation violates the
indentation convention used in the rest of the file.  That could
be fixed by saying

  .de1 PD
  .  nop \\*[an-reset-paragraph-spacing]\\
  ..

but i wouldn't consider that sufficiently readable either.

So you are obfuscating what is going on by

 1. Defining a macro the supports an optional argument without
    any argument expansion ever appearing in the definition of the
    macro (PD);

 2. using string expansion that actually amounts to a macro call
    (\\*[an-reset-paragraph-spacing]);

 3. and also hiding the fact from the reader of the code that an
    optional argument is being passed by the macro call
    \\*[an-reset-paragraph-spacing].

I searched for this kind of obfuscation anywhere else in an-old.tmac,
but all the other instances of lines starting with \\* or .nop \\
actually expand strings rather than calling macros.

If you want to call a macro, please do write it as a macro call,
not as string expansion.  For passing variable numbers of arguments,
the escape sequences \\$* and \\$@ are available, and sometimes,
even just using \\$1 for a single optional argument may be good
enough, because in many situations, appending a space character
and an empty string to a macro line changes nothing, not even \\n[.$].

Finally, i'm not sure why new macros are needed at all.
Even if you want to add deprecation diagnostics, why not just insert
the required code into the existing macros?

Yours,
  Ingo