| Message ID | 1318881685-23983-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 17, 2011 at 10:01:25PM +0200, Stefan Weil wrote: > > The patch also slightly cleans the g_malloc0 statement which was > touched by that change (no type cast, easier code review). [...] > - s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState)); [...] > + s = g_malloc0(sizeof(*s)); Is this the preferred style, or should we be using: s = g_new0(GDBRegisterState, 1); We currently seem to have a mix of the two styles. I kinda prefer using g_new() (or g_new0()) since provided the 'count' parameter is correct, it can't really be wrong, whereas using sizeof() has the potential to be buggy, even if it is still trivial to check that the code is okay. I guess I feel g_malloc() would be best reserved for those cases where we're doing something special which might need a little extra care... BTW, I've just been experimenting with Coccinelle and produced the following semantic patch: @@ type T; @@ -(T *)g_malloc(sizeof(T)) +g_new(T, 1) @@ type T; @@ -(T *)g_malloc0(sizeof(T)) +g_new0(T, 1) @@ type T; expression E; @@ -(T *)g_malloc(sizeof(T) * E) +g_new(T, E) @@ type T; expression E; @@ -(T *)g_malloc0(sizeof(T) * E) +g_new0(T, E) I couldn't get the following working: // THIS DOES NOT WORK: @@ type T; identifier I; @@ -T I = g_malloc(sizeof(*I)) +T I = g_new(T, 1) I expect this won't work either: // THIS PROBABLY DOES NOT WORK EITHER: @@ type T; identifier I; @@ -T I = g_new(T, 1) +T I = g_malloc(sizeof(*I)) Cheers,
On Tue, Oct 18, 2011 at 1:13 AM, Stuart Brady <sdb@zubnet.me.uk> wrote: > On Mon, Oct 17, 2011 at 10:01:25PM +0200, Stefan Weil wrote: >> >> The patch also slightly cleans the g_malloc0 statement which was >> touched by that change (no type cast, easier code review). > [...] >> - s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState)); > [...] >> + s = g_malloc0(sizeof(*s)); > > Is this the preferred style, or should we be using: > > s = g_new0(GDBRegisterState, 1); > > We currently seem to have a mix of the two styles. > > I kinda prefer using g_new() (or g_new0()) since provided the 'count' > parameter is correct, it can't really be wrong, whereas using sizeof() > has the potential to be buggy, even if it is still trivial to check > that the code is okay. > > I guess I feel g_malloc() would be best reserved for those cases where > we're doing something special which might need a little extra care... > > BTW, I've just been experimenting with Coccinelle and produced the > following semantic patch: > > @@ type T; @@ > -(T *)g_malloc(sizeof(T)) > +g_new(T, 1) > > @@ type T; @@ > -(T *)g_malloc0(sizeof(T)) > +g_new0(T, 1) > > @@ type T; expression E; @@ > -(T *)g_malloc(sizeof(T) * E) > +g_new(T, E) > > @@ type T; expression E; @@ > -(T *)g_malloc0(sizeof(T) * E) > +g_new0(T, E) Cool. Please include the spatch with the commit message. > I couldn't get the following working: > > // THIS DOES NOT WORK: > @@ type T; identifier I; @@ > -T I = g_malloc(sizeof(*I)) > +T I = g_new(T, 1) > > I expect this won't work either: > > // THIS PROBABLY DOES NOT WORK EITHER: > @@ type T; identifier I; @@ > -T I = g_new(T, 1) > +T I = g_malloc(sizeof(*I)) IIRC I had also problems with identifiers, I could not get checking identifiers against CODING_STYLE to work.
On Tue, Oct 18, 2011 at 06:18:11PM +0000, Blue Swirl wrote: > Cool. Please include the spatch with the commit message. Thanks, will do! FWIW, it results in: 51 files changed, 99 insertions(+), 136 deletions(-) I wonder if that needs splitting up at all? > IIRC I had also problems with identifiers, I could not get checking > identifiers against CODING_STYLE to work. FWIW, I got there in the end: @@ type T; T *E; @@ -E = g_malloc(sizeof(*E)) +E = g_new(T, 1) @@ type T; T *E; @@ -E = g_malloc0(sizeof(*E)) +E = g_new0(T, 1) @@ type T; T *E; expression N; @@ -E = g_malloc(sizeof(*E) * N) +E = g_new(T, N) @@ type T; T *E; expression N; @@ -E = g_malloc0(sizeof(*E) * N) +E = g_new0(T, N) @@ type T; T *E; @@ -E = g_malloc(sizeof(T)) +E = g_new(T, 1) @@ type T; T *E; @@ -E = g_malloc0(sizeof(T)) +E = g_new0(T, 1) @@ type T; T *E; expression N; @@ -E = g_malloc(sizeof(T) * N) +E = g_new(T, N) @@ type T; T *E; expression N; @@ -E = g_malloc0(sizeof(T) * N) +E = g_new0(T, N) With this added, I get: 246 files changed, 514 insertions(+), 557 deletions(-) That includes the original 99 insertions and 136 deletions. I think I should at least submit the patch to convert cases involving casts as the first part of the patch series, and convert the sizeof(*E) and sizeof(T) cases as the second and third parts. Sound okay? Cheers,
On Wed, Oct 19, 2011 at 01:59:04AM +0100, Stuart Brady wrote: > On Tue, Oct 18, 2011 at 06:18:11PM +0000, Blue Swirl wrote: > > > Cool. Please include the spatch with the commit message. > > Thanks, will do! Okay, submitted. This is the first time I've used git send-email, so let me know if I've missed anything. Cheers,
On Thu, Oct 20, 2011 at 08:10, Stuart Brady <sdb@zubnet.me.uk> wrote: > On Wed, Oct 19, 2011 at 01:59:04AM +0100, Stuart Brady wrote: >> On Tue, Oct 18, 2011 at 06:18:11PM +0000, Blue Swirl wrote: >> >> > Cool. Please include the spatch with the commit message. >> >> Thanks, will do! > > Okay, submitted. > > This is the first time I've used git send-email, so let me know if I've > missed anything. The patches are OK, though when sending multiple patches, a cover letter (--cover-letter) is appreciated.
diff --git a/gdbstub.c b/gdbstub.c index 4009058..34746f2 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1768,12 +1768,6 @@ void gdb_register_coprocessor(CPUState * env, GDBRegisterState **p; static int last_reg = NUM_CORE_REGS; - s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState)); - s->base_reg = last_reg; - s->num_regs = num_regs; - s->get_reg = get_reg; - s->set_reg = set_reg; - s->xml = xml; p = &env->gdb_regs; while (*p) { /* Check for duplicates. */ @@ -1781,6 +1775,14 @@ void gdb_register_coprocessor(CPUState * env, return; p = &(*p)->next; } + + s = g_malloc0(sizeof(*s)); + s->base_reg = last_reg; + s->num_regs = num_regs; + s->get_reg = get_reg; + s->set_reg = set_reg; + s->xml = xml; + /* Add to end of list. */ last_reg += num_regs; *p = s;
cppcheck report: gdbstub.c:1781: error: Memory leak: s Rearranging of the code avoids the leak. The patch also slightly cleans the g_malloc0 statement which was touched by that change (no type cast, easier code review). Signed-off-by: Stefan Weil <sw@weilnetz.de> --- gdbstub.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)