gdb: remove final m_stream->emit_style_escape calls from pager_file

After the last commit there were still a couple of calls to
m_stream->emit_style_escape in the pager_file class.  As discussed in
the last commit, these are likely wrong, but I'd not been able to
produce any bugs because of them.

The reason why there are no bugs is that these calls are, I think,
entirely redundant.  Consider this block:

      if (m_wrap_column)
	{
	  /* We are about to insert a newline at an historic
	     location in the WRAP_BUFFER.  Before we do we want to
	     restore the default style.  To know if we actually
	     need to insert an escape sequence we must restore the
	     current applied style to how it was at the WRAP_COLUMN
	     location.  */
	  m_applied_style = m_wrap_style;
	  m_stream->emit_style_escape (ui_file_style ());
	  /* If we aren't actually wrapping, don't output
	     newline -- if chars_per_line is right, we
	     probably just overflowed anyway; if it's wrong,
	     let us keep going.  */
	  m_stream->puts ("\n");
	}

What we know (see previous commit) is that the call:

  m_stream->emit_style_escape (ui_file_style ());

is dangerous as m_stream->m_applied_style is going to be out of sync
with its current state.  Actually, m_stream->m_applied_style is likely
to be the default style as it is not updated elsewhere.  So why does
this not cause problems?

Well, GDB's style output is always done in tightly scoped regions.
That means if we want to print some styled output, and then apply a
wrap point the code might look like this:

  fprintf_styled (gdb_stdout, file_name_style, "some text");
  gdb_stdout->wrap_here (4);

But, after printing 'some text', the style of gdb_stdout will have
returned to the default style.

My claim is that, whenever we encounter a wrap_here call, the stream
in question will _always_ have been returned to the default style.

This means that, in the block above, the call:

  m_stream->emit_style_escape (ui_file_style ());

will never emit anything because it depends on a check against
m_stream->m_applied_style, which will always mean that the above call
does nothing.  But that's OK.  By chance, we'll have always placed the
stream into a default style state anyway, so no harm done.

Similarly, the other call:

  /* Having finished inserting the wrapping we should
     restore the style as it was at the WRAP_COLUMN.  */
  m_stream->emit_style_escape (m_wrap_style);

Tries to return m_stream to the state it was in at the point of the
wrap_here call.  But, as described above, this will always be the
default style, so the above call will do nothing, but that just
happens to be exactly what we want!

So what does this commit do?

Well, I "fix" the above code by removing the
m_stream->emit_style_escape calls and replacing them with calls to
puts, passing in the escape sequence for the required style, but only
if the m_stream style as tracked by pager_file::m_stream_style
indicates this is needed.

Got the reasons given above, this should mean there is no change after
this patch.  We still shouldn't be emitting any extra escape
sequences.  But, should we ever manage to get into a state where we
call wrap_here with a stream in a style other than the default, then
this should mean things work as expected.

There should be no user visible changes after this commit.

Approved-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/pager.h b/gdb/pager.h
index 9fbb310d0d0c94f1bda9642f9b24deeeb30be202..b5425d4e1e24b5e69ec8562ed823f7402ac5a5a2 100644 (file)
--- a/gdb/pager.h
+++ b/gdb/pager.h
@@ -68,6 +68,16 @@ private:
   /* Flush the wrap buffer to STREAM, if necessary.  */
   void flush_wrap_buffer ();
 
+  /* Set the style of m_stream to STYLE.  */
+  void set_stream_style (const ui_file_style &style)
+  {
+    if (m_stream->can_emit_style_escape () && m_stream_style != style)
+      {
+       m_stream->puts (style.to_ansi ().c_str ());
+       m_stream_style = style;
+      }
+  }
+
   /* Contains characters which are waiting to be output (they have
      already been counted in chars_printed).  */
   std::string m_wrap_buffer;

diff --git a/gdb/utils.c b/gdb/utils.c
index e71f1b962f35cc2931d71738f716664c96a05225..6ae362c54001ea3c62ed99b0f058b755c251f238 100644 (file)
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1419,11 +1419,7 @@ pager_file::emit_style_escape (const ui_file_style &style)
             the style we are applying matches what we know is currently
             applied to the underlying stream, then we can skip sending
             this style to the stream.  */
-         if (m_stream_style != m_applied_style)
-           {
-             m_stream->puts (style.to_ansi ().c_str ());
-             m_stream_style = m_applied_style;
-           }
+         this->set_stream_style (m_applied_style);
        }
       else
        m_wrap_buffer.append (style.to_ansi ());
@@ -1748,7 +1744,8 @@ pager_file::puts (const char *linebuffer)
                     current applied style to how it was at the WRAP_COLUMN
                     location.  */
                  m_applied_style = m_wrap_style;
-                 m_stream->emit_style_escape (ui_file_style ());
+                 this->set_stream_style (ui_file_style ());
+
                  /* If we aren't actually wrapping, don't output
                     newline -- if chars_per_line is right, we
                     probably just overflowed anyway; if it's wrong,
@@ -1776,7 +1773,7 @@ pager_file::puts (const char *linebuffer)
 
                  /* Having finished inserting the wrapping we should
                     restore the style as it was at the WRAP_COLUMN.  */
-                 m_stream->emit_style_escape (m_wrap_style);
+                 this->set_stream_style (m_wrap_style);
 
                  /* The WRAP_BUFFER will still contain content, and that
                     content might set some alternative style.  Restore