This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy
On 2017-01-23 18:18, Pedro Alves wrote:
@@ -195,20 +188,18 @@ handle_redirections (int from_tty)
return;
}
- output = gdb_fopen (logging_filename, logging_overwrite ? "w" :
"a");
- if (output == NULL)
+ std::unique_ptr<stdio_file> log (new stdio_file ());
What is the "rule" for using std::unique_ptr directly vs defining a
typedef?
typedef std::unique_ptr<stdio_file> stdio_file_up;
I'd like if we could avoid having a mix of styles, unless there are good
reasons for it.
+ if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
perror_with_name (_("set logging"));
- cleanups = make_cleanup_ui_file_delete (output);
+
+ output = log.get ();
/* Redirects everything to gdb_stdout while this is running. */
if (!logging_redirect)
{
no_redirect_file = output;
- output = tee_file_new (gdb_stdout, 0, no_redirect_file, 0);
- if (output == NULL)
- perror_with_name (_("set logging"));
- make_cleanup_ui_file_delete (output);
+ output = new tee_file (gdb_stdout, 0, no_redirect_file, 0);
Is there anything that replaces this cleanup? IOW, if the
fprinf_unfiltered just below did throw an exception for some reason,
would this tee_file leak?
if (from_tty)
fprintf_unfiltered (gdb_stdout, "Copying output to %s.\n",
logging_filename);
...
@@ -420,22 +412,20 @@ c_compute_program (struct compile_instance *inst,
? "&" : ""));
break;
default:
- fputs_unfiltered (input, buf);
+ buf.puts (input);
break;
}
- fputs_unfiltered ("\n", buf);
+ buf.puts ("\n");
/* For larger user expressions the automatic semicolons may be
confusing. */
if (strchr (input, '\n') == NULL)
- fputs_unfiltered (";\n", buf);
+ buf.puts (";\n");
if (inst->scope != COMPILE_I_RAW_SCOPE)
- fputs_unfiltered ("}\n", buf);
+ buf.puts ("}\n");
- add_code_footer (inst->scope, buf);
- code = ui_file_as_string (buf);
- do_cleanups (cleanup);
- return code;
+ add_code_footer (inst->scope, &buf);
+ return std::move (buf.string ());
I would have thought that this std::move would be superfluous, because
the compiler would do it anyway. Is it the case? Is it a good practice
to use move explicitly to make sure it's a move and not a copy (and
probably get a compile-time error if a move is not possible)?
diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
index d06c481..bf3c424 100644
--- a/gdb/guile/scm-disasm.c
+++ b/gdb/guile/scm-disasm.c
@@ -162,7 +162,7 @@ gdbscm_disasm_print_address (bfd_vma addr, struct
disassemble_info *info)
static int
gdbscm_print_insn_from_port (struct gdbarch *gdbarch,
SCM port, ULONGEST offset, CORE_ADDR memaddr,
- struct ui_file *stream, int *branch_delay_insns)
+ string_file *stream, int *branch_delay_insns)
Reference instead of pointer? I don't really mind, it's just that
you've used references in other places (e.g. the compile code) when
changing a ui_file pointer to a more specific type.
diff --git a/gdb/guile/scm-ports.c b/gdb/guile/scm-ports.c
index 4a1c864..fb3a47b 100644
--- a/gdb/guile/scm-ports.c
+++ b/gdb/guile/scm-ports.c
@@ -37,11 +37,18 @@
/* A ui-file for sending output to Guile. */
-typedef struct
+class ioscm_file_port : public ui_file
{
- int *magic;
- SCM port;
-} ioscm_file_port;
+public:
+ /* Return a ui_file that writes to PORT. */
+ explicit ioscm_file_port (SCM port);
+
+ void flush () override;
+ void write (const char *buf, long length_buf) override;
+
+private:
+ SCM m_port;
+};
/* Data for a memory port. */
@@ -431,74 +438,21 @@ gdbscm_error_port (void)
/* Support for sending GDB I/O to Guile ports. */
-static void
-ioscm_file_port_delete (struct ui_file *file)
-{
- ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
-
- if (stream->magic != &file_port_magic)
- internal_error (__FILE__, __LINE__,
- _("ioscm_file_port_delete: bad magic number"));
- xfree (stream);
-}
-
-static void
-ioscm_file_port_rewind (struct ui_file *file)
-{
- ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
-
- if (stream->magic != &file_port_magic)
- internal_error (__FILE__, __LINE__,
- _("ioscm_file_port_rewind: bad magic number"));
-
- scm_truncate_file (stream->port, 0);
-}
+ioscm_file_port::ioscm_file_port (SCM port)
+ : m_port (port)
+{}
-static void
-ioscm_file_port_put (struct ui_file *file,
- ui_file_put_method_ftype *write,
- void *dest)
+void
+ioscm_file_port::flush ()
{
- ioscm_file_port *stream = (ioscm_file_port *) ui_file_data (file);
-
- if (stream->magic != &file_port_magic)
- internal_error (__FILE__, __LINE__,
- _("ioscm_file_port_put: bad magic number"));
-
- /* This function doesn't meld with ports very well. */
}
It looks like there's a low-level function, scm_force_output, that
"flushes" a port. I wonder if we'd want to do that in flush.
diff --git a/gdb/mi/mi-console.c b/gdb/mi/mi-console.c
index afb5e94..218ffbc 100644
--- a/gdb/mi/mi-console.c
+++ b/gdb/mi/mi-console.c
@@ -26,118 +26,62 @@
#include "defs.h"
#include "mi-console.h"
-static ui_file_fputs_ftype mi_console_file_fputs;
-static ui_file_flush_ftype mi_console_file_flush;
-static ui_file_delete_ftype mi_console_file_delete;
-
-struct mi_console_file
- {
- int *magic;
- struct ui_file *raw;
- struct ui_file *buffer;
- const char *prefix;
- char quote;
- };
-
-/* Use the address of this otherwise-unused global as a magic number
- identifying this class of ui_file objects. */
-static int mi_console_file_magic;
/* Create a console that wraps the given output stream RAW with the
string PREFIX and quoting it with QUOTE. */
-struct ui_file *
-mi_console_file_new (struct ui_file *raw, const char *prefix, char
quote)
-{
- struct ui_file *ui_file = ui_file_new ();
- struct mi_console_file *mi_console = XNEW (struct mi_console_file);
-
- mi_console->magic = &mi_console_file_magic;
- mi_console->raw = raw;
- mi_console->buffer = mem_fileopen ();
- mi_console->prefix = prefix;
- mi_console->quote = quote;
- set_ui_file_fputs (ui_file, mi_console_file_fputs);
- set_ui_file_flush (ui_file, mi_console_file_flush);
- set_ui_file_data (ui_file, mi_console, mi_console_file_delete);
-
- return ui_file;
-}
+mi_console_file::mi_console_file (ui_file *raw, const char *prefix,
char quote)
+ : m_raw (raw),
+ m_prefix (prefix),
+ m_quote (quote)
+{}
-static void
-mi_console_file_delete (struct ui_file *file)
+void
+mi_console_file::puts (const char *buf)
{
- struct mi_console_file *mi_console
- = (struct mi_console_file *) ui_file_data (file);
-
- if (mi_console->magic != &mi_console_file_magic)
- internal_error (__FILE__, __LINE__,
- _("mi_console_file_delete: bad magic number"));
-
- xfree (mi_console);
+ this->write (buf, strlen (buf));
}
mi_console_file::puts looks identical to ui_file::puts. Is there a
reason to overload it?
@@ -1516,15 +1509,13 @@ mi_cmd_data_read_memory (char *command, char
**argv, int argc)
/* Build the result as a two dimentional table. */
{
- struct ui_file *stream;
- struct cleanup *cleanup_stream;
int row;
int row_byte;
+ struct cleanup *cleanup_list;
- stream = mem_fileopen ();
- cleanup_stream = make_cleanup_ui_file_delete (stream);
+ string_file stream;
- make_cleanup_ui_out_list_begin_end (uiout, "memory");
+ cleanup_list = make_cleanup_ui_out_list_begin_end (uiout,
"memory");
for (row = 0, row_byte = 0;
row < nr_rows;
row++, row_byte += nr_cols * word_size)
Throughout mi_cmd_data_read_memory, is is worth it performance-wise to
re-use the same string_file and doing some .clear() rather than
declaring different ones in more specific scopes? Since they are stack
allocated, there shouldn't be any runtime cost in execution time. Which
is faster between constructing a string_file and calling .clear()?
I think it would be less error-prone (like forgetting to call .clear()
between two usages) to declare multiple string_file objects in the
scopes that need them, but I understand if you'd rather not do it for
performance reasons. I also understand if you say that it's because
it's the way it worked and don't want to spend time on a deprecated
function anyway :).
/* Expand tabs into spaces, since ncurses on MS-Windows doesn't. */
- ret = tui_expand_tabs (p, 0);
+ ret = tui_expand_tabs (str.c_str (), 0);
It's not in the scope of this patch, but it would be nice to rewrite
tui_expand_tabs using C++ features, I'm sure it could be much simpler.
I'm not done looking at the patch yet, but I've deleted to much of the
quote and I have comments about stuff I deleted, so it will be simpler
to just send another e-mail.