Re: [PATCH] macro hooks

[Timothy Wall <twall at alum dot mit dot edu>]

Ian Lance Taylor wrote:

>    Date: Wed, 23 Feb 2000 11:16:45 -0500
>    From: Timothy Wall <twall@alum.mit.edu>
>
>
>    --- 259,279 ----
>           sb *from;
>           char *position;
>      {
>    !   /* I put this flag so that it's possible to do a wholesale
>    !      replace of a single line w/o disrupting the input stream; if the checking
>    !      is enabled, substitutions on lines like " .if SUBS" will fail, thinking
>    !      they have to see ".endif" before the end of the single-line sb
>    !      twall@cygnus.com
>    !   */
>    !   if (!from->no_macro_check)
>    !     {
>    !       if (macro_nest > max_macro_nest)
>    !         as_fatal (_("macros nested too deeply"));
>    !       ++macro_nest;
>    ! #ifdef md_macro_start
>    !       md_macro_start ();
>    ! #endif
>    !     }
>
>        next_saved_file = input_scrub_push (position);
>
> I don't like this approach.  struct sb is a simple structure that
> represents a line in memory.  I think that adding a field like
> no_macro_check is inappropriately mixing levels.  struct sb is not
> about macros.  It is about strings stored in memory.

  In general, I'm trying to get things so I'm able to substitute tokens
on a given line.   One way of doing this is to suck down the line, perform
the substitutions, and then parse the replacement line.  The flag handles
a special case like the following:

    .asg SUB_TOKEN, ARG > 0
    .if SUB_TOKEN

gets replaced by

    .if ARG > 0

  The sb handling expects an entire conditional clause (or macro definition)
to be contained within a single sb; if the the line above is in its own sb,
the conditional checking reaches the end of the line's sb and complains b/c
it hasn't seen the endif before the end of sb.  Perhaps there is a different
way of modifying the sb contents to avoid this issue.

  The push and pop don't cleanly map b/c the original line is being replaced
rather than a new line of different input inserted.


>
>
> As far as I can tell, you are adding this flag to avoid calling your
> new macros, and to avoid calling cond_finish_check.  That is
> reasonable, but the place to store this information is in a new
> variable which is saved and restored by input_scrub_push and
> input_scrub_pop.  This variable could then be set by
> input_scrub_include_sb, or perhaps even by a new function which called
> input_scrub_include_sb.
>
> Note that I don't see why this new flag should control macro_nest.
> macro_nest seems just as appropriate for a single line as for an
> entire macro.  I do see why it should control cond_finish_check.
>
> Also, this is a minor point, but I think it's bad style to name flags
> in the negative, like no_macro_check.  That makes it harder to
> understand how to use them correctly.  Name the flag positively,
> macro_check, and reverse the uses.

>
>      int
>    ! check_macro (line, expand, comment_char, error, info)
>           const char *line;
>           sb *expand;
>           int comment_char;
>           const char **error;
>    +      void **info;
>
> What is the point of this?  Nobody can use the returned information.
> If you need to add a parameter like this, don't you need to move the
> struct definition into macro.h?  Even if you don't, there is no need
> to lose the type information.  The info parameter should be struct
> macro_struct **, not void **.
>

Yes, the macro_struct should be exported.  I avoided doing so in the
initial implementation to avoid affecting too many files.

>
>    +   /* export the macro information if requested */
>
> Comments should be complete sentences with correct capitalization and
> punctuation.
>
>    +   sb newline;
>    +   sb_new (&newline);
>    +   while (*line != 0)
>    +     sb_add_char (&newline, *line++);
>
> Use sb_add_string.
>
>    + /* Insert a (usually automatically generated) file into the input stream;
>    +    the path must resolve to an actual file; no include path searching or
>    +    dependency registering is performed.
>    +  */
>
> The */ at the end of a comment should go on the same line as the last
> sentence in the comment.  I think you did this a few other places as
> well.
>
>    + void
>    + insert_file (path)
>    +   const char *path;
>
> Perhaps s_include should be changed to call insert_file.
>
> I think input_scrub_insert_file might be a better name.  Also
> input_scrub_insert_line.  insert_line and insert_file are a trifle too
> generic.
>
> Ian

Good point.   They would have gone into input-scrub.c, except that
they both need to modify the static buffer_limit in read.c.

T.