Re: Commit: Add support for a temporary input line pointer in gas/read.c

[Alan Modra <amodra at gmail dot com>]

On Fri, Jan 13, 2017 at 09:58:03AM +0000, Nick Clifton wrote:
> Hi Alan,
> 
> >> +static char *saved_ilp = NULL;
> >> +static char *saved_limit;
> > 
> > Can I suggest instead that you declare a struct save_ilp, use one of
> > them as a local var, and pass that by reference to the save/restore
> > functions.  They could be made inline too.
> 
> I could do this, but why ?  What do we gain.  It is not like these functions
> need to be fast, or that they affect the performance of the assembler.

We gain:
a) Less global state.  In this case, possibly doesn't affect things
   like https://sourceware.org/ml/binutils/2015-06/msg00010.html, but
   global state without good reason ought to be avoided.  Yes, you
   can tell me that next time I add some. :)
b) Reentrancy.  Do we save input_line_pointer and set it to a string
   somewhere then do the same again in some function called?  Only
   matters of course if you want to write a bit of generally useful
   infrastructure that might be used is other places.
c) Local state means asserts and setting saved_ilp NULL after use is
   hardly necessary.

> >> +  /* Prevent the assert in restore_ilp from triggering if
> >> +     the input_line_pointer has not yet been initialised.  */
> >> +  if (saved_ilp == NULL)
> >> +    saved_limit = saved_ilp = (char *) "";
> > 
> > Changing saved_ilp means it really isn't saved!
> 
> True, but I just wanted to avoid having to create a third local variable
> saying 'the saved_ilp has been initialised'.  It does not matter that we
> "restore" the input_line_pointer to an empty buffer, because the only way
> that this situation can arise is if the input_line_pointer has not yet
> been used.

If I had added the assert and I found it triggered on testing due to
input_line_pointer validly being NULL, I'd decide that I didn't really
need the assert!

-- 
Alan Modra
Australia Development Lab, IBM