[RFA,14/22] Replace two xmallocs with vector
Commit Message
This replaces a couple of uses of xmalloc with a std::vector, also
removing a couple of cleanups.
2016-09-26 Tom Tromey <tom@tromey.com>
* cli/cli-dump.c (dump_memory_to_file): Use std::vector.
(restore_binary_file): Likewise.
---
gdb/ChangeLog | 5 +++++
gdb/cli/cli-dump.c | 21 +++++++++------------
2 files changed, 14 insertions(+), 12 deletions(-)
Comments
On 09/27/2016 05:08 AM, Tom Tromey wrote:
> This replaces a couple of uses of xmalloc with a std::vector, also
> removing a couple of cleanups.
>
> 2016-09-26 Tom Tromey <tom@tromey.com>
>
> * cli/cli-dump.c (dump_memory_to_file): Use std::vector.
> (restore_binary_file): Likewise.
As general guideline, for these cases where we only need to
construct a buffer once (never resize/reallocate) and we don't
care about the initial contents of the buffer, I think
unique_ptr<char[]> buf (new char[size]);
ends up being more efficient, because std::vector
default/zero initializes its elements, which is unnecessary since
we're about to write into the buffer anyway.
WDYT?
(I'll try to post my unique_ptr shim soon.)
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> * cli/cli-dump.c (dump_memory_to_file): Use std::vector.
>> (restore_binary_file): Likewise.
Pedro> As general guideline, for these cases where we only need to
Pedro> construct a buffer once (never resize/reallocate) and we don't
Pedro> care about the initial contents of the buffer, I think
Pedro> unique_ptr<char[]> buf (new char[size]);
Pedro> ends up being more efficient, because std::vector
Pedro> default/zero initializes its elements, which is unnecessary since
Pedro> we're about to write into the buffer anyway.
Pedro> WDYT?
It's fine with me.
Often the performance doesn't matter, and std::vector is safe to use.
On the other hand XNEWVEC isn't really unsafe -- maybe just mildly less
clear to gdb newbies.
Tom
On 10/12/2016 11:39 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
>>> * cli/cli-dump.c (dump_memory_to_file): Use std::vector.
>>> (restore_binary_file): Likewise.
>
> Pedro> As general guideline, for these cases where we only need to
> Pedro> construct a buffer once (never resize/reallocate) and we don't
> Pedro> care about the initial contents of the buffer, I think
> Pedro> unique_ptr<char[]> buf (new char[size]);
> Pedro> ends up being more efficient, because std::vector
> Pedro> default/zero initializes its elements, which is unnecessary since
> Pedro> we're about to write into the buffer anyway.
>
> Pedro> WDYT?
>
> It's fine with me.
>
> Often the performance doesn't matter, and std::vector is safe to use.
> On the other hand XNEWVEC isn't really unsafe -- maybe just mildly less
> clear to gdb newbies.
Hmm, the code you're touching isn't using XNEWVEC, so I'm not sure
I know exactly what you're referring to, but I assume you're thinking
of the xmallocs you're replacing.
Both vector and unique_ptr<T[]> (or unique_malloc_ptr<T> with the
to go with the existing mallocs) have the advantage of not requiring a
cleanup, of course. Writing either I think is almost the same code.
I think unique_ptr communicates intent a little better for these
cases, with the added bonus of a little extra efficiency, but it's not
a huge issue.
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> Often the performance doesn't matter, and std::vector is safe to use.
>> On the other hand XNEWVEC isn't really unsafe -- maybe just mildly less
>> clear to gdb newbies.
Pedro> Hmm, the code you're touching isn't using XNEWVEC, so I'm not sure
Pedro> I know exactly what you're referring to, but I assume you're thinking
Pedro> of the xmallocs you're replacing.
Yeah.
Hmm, if it's xmalloc (I'm looking at one patch where it's XCNEWVEC, but
apparently could be XNEWVEC without any loss), then I'm less sure.
XNEWVEC is at least reasonably type-safe, but plain xmalloc less so.
Anyway once unique_ptr is in I'll update all those patches and we can
discuss it more concretely.
Tom
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> As general guideline, for these cases where we only need to
Pedro> construct a buffer once (never resize/reallocate) and we don't
Pedro> care about the initial contents of the buffer, I think
Pedro> unique_ptr<char[]> buf (new char[size]);
Pedro> ends up being more efficient, because std::vector
Pedro> default/zero initializes its elements, which is unnecessary since
Pedro> we're about to write into the buffer anyway.
I made this change in my patches.
While doing so I realized one reason to prefer vector: with vector you
can build gdb with the libstdc++ debug mode, and get range checking.
Tom
On Thu, Oct 13, 2016 at 08:51:13AM -0600, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> As general guideline, for these cases where we only need to
> Pedro> construct a buffer once (never resize/reallocate) and we don't
> Pedro> care about the initial contents of the buffer, I think
> Pedro> unique_ptr<char[]> buf (new char[size]);
> Pedro> ends up being more efficient, because std::vector
> Pedro> default/zero initializes its elements, which is unnecessary since
> Pedro> we're about to write into the buffer anyway.
>
> I made this change in my patches.
>
> While doing so I realized one reason to prefer vector: with vector you
> can build gdb with the libstdc++ debug mode, and get range checking.
On the other hand if there isn't already a debug unique_ptr with the same
checking it seems like we should add that. It shouldn't be terribly
hard.
Trev
>
> Tom
@@ -1,5 +1,10 @@
2016-09-26 Tom Tromey <tom@tromey.com>
+ * cli/cli-dump.c (dump_memory_to_file): Use std::vector.
+ (restore_binary_file): Likewise.
+
+2016-09-26 Tom Tromey <tom@tromey.com>
+
* stabsread.c (read_struct_type): Remove unnecessary cleanup.
2016-09-26 Tom Tromey <tom@tromey.com>
@@ -31,7 +31,7 @@
#include "cli/cli-utils.h"
#include "gdb_bfd.h"
#include "filestuff.h"
-
+#include <vector>
static const char *
scan_expression_with_cleanup (const char **cmd, const char *def)
@@ -212,7 +212,6 @@ dump_memory_to_file (const char *cmd, const char *mode, const char *file_format)
CORE_ADDR hi;
ULONGEST count;
const char *filename;
- gdb_byte *buf;
const char *lo_exp;
const char *hi_exp;
@@ -237,18 +236,17 @@ dump_memory_to_file (const char *cmd, const char *mode, const char *file_format)
/* FIXME: Should use read_memory_partial() and a magic blocking
value. */
- buf = (gdb_byte *) xmalloc (count);
- make_cleanup (xfree, buf);
- read_memory (lo, buf, count);
+ std::vector<gdb_byte> buf (count);
+ read_memory (lo, buf.data (), count);
/* Have everything. Open/write the data. */
if (file_format == NULL || strcmp (file_format, "binary") == 0)
{
- dump_binary_file (filename, mode, buf, count);
+ dump_binary_file (filename, mode, buf.data (), count);
}
else
{
- dump_bfd_file (filename, mode, file_format, lo, buf, count);
+ dump_bfd_file (filename, mode, file_format, lo, buf.data (), count);
}
do_cleanups (old_cleanups);
@@ -518,7 +516,6 @@ restore_binary_file (const char *filename, struct callback_data *data)
{
struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
FILE *file = fopen_with_cleanup (filename, FOPEN_RB);
- gdb_byte *buf;
long len;
/* Get the file size for reading. */
@@ -553,13 +550,13 @@ restore_binary_file (const char *filename, struct callback_data *data)
perror_with_name (filename);
/* Now allocate a buffer and read the file contents. */
- buf = (gdb_byte *) xmalloc (len);
- make_cleanup (xfree, buf);
- if (fread (buf, 1, len, file) != len)
+ std::vector<gdb_byte> buf (len);
+ if (fread (buf.data (), 1, len, file) != len)
perror_with_name (filename);
/* Now write the buffer into target memory. */
- len = target_write_memory (data->load_start + data->load_offset, buf, len);
+ len = target_write_memory (data->load_start + data->load_offset,
+ buf.data (), len);
if (len != 0)
warning (_("restore: memory write failed (%s)."), safe_strerror (len));
do_cleanups (cleanup);