From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119936 invoked by alias); 19 Oct 2016 23:58:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 119926 invoked by uid 89); 19 Oct 2016 23:58:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=XCNEWVEC, xcnewvec, 3730, agent X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Oct 2016 23:58:04 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 781048553E; Wed, 19 Oct 2016 23:58:03 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9JNw2fI010513; Wed, 19 Oct 2016 19:58:02 -0400 Subject: Re: [PATCH v2 29/31] 'struct agent_expr *' -> unique_ptr To: Simon Marchi References: <1476839539-8374-1-git-send-email-palves@redhat.com> <1476839539-8374-30-git-send-email-palves@redhat.com> <57020f6d8d6c087cc8ea35f49860e7d8@simark.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <3cfca740-279c-5ade-23b6-14a7dc94e953@redhat.com> Date: Wed, 19 Oct 2016 23:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <57020f6d8d6c087cc8ea35f49860e7d8@simark.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00595.txt.bz2 On 10/20/2016 12:19 AM, Simon Marchi wrote: > On 2016-10-18 21:12, Pedro Alves wrote: >> diff --git a/gdb/ax-general.c b/gdb/ax-general.c >> index 7f27a45..35225f6 100644 >> --- a/gdb/ax-general.c >> +++ b/gdb/ax-general.c >> @@ -37,52 +37,30 @@ static void generic_ext (struct agent_expr *x, >> enum agent_op op, int n); >> >> /* Functions for building expressions. */ >> >> -/* Allocate a new, empty agent expression. */ >> -struct agent_expr * >> -new_agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope) >> +agent_expr::agent_expr (struct gdbarch *gdbarch, CORE_ADDR scope) >> { >> - struct agent_expr *x = XNEW (struct agent_expr); >> - >> - x->len = 0; >> - x->size = 1; /* Change this to a larger value once >> + this->len = 0; >> + this->size = 1; /* Change this to a larger value once >> reallocation code is tested. */ >> - x->buf = (unsigned char *) xmalloc (x->size); >> + this->buf = (unsigned char *) xmalloc (this->size); >> >> - x->gdbarch = gdbarch; >> - x->scope = scope; >> + this->gdbarch = gdbarch; >> + this->scope = scope; >> >> /* Bit vector for registers used. */ >> - x->reg_mask_len = 1; >> - x->reg_mask = XCNEWVEC (unsigned char, x->reg_mask_len); >> - >> - x->tracing = 0; >> - x->trace_string = 0; >> + this->reg_mask_len = 1; >> + this->reg_mask = XCNEWVEC (unsigned char, this->reg_mask_len); >> >> - return x; >> + this->tracing = 0; >> + this->trace_string = 0; >> } > > In one of Tom's patches, you said to drop the "this->". Did you leave > them here for clarity? Would you remove them if the structure was > completely converted, with m_ prefixed members (given that the m_ prefix > makes it clear enough that it's a member)? m_ is supposed to used for private data fields. In this case, the fields are still public (and referenced from outside the class in many places). I think in such cases using this-> makes the code much clearer. > >> @@ -12385,13 +12378,9 @@ force_breakpoint_reinsertion (struct >> bp_location *bl) >> that have already been marked. */ >> loc->condition_changed = condition_updated; >> >> - /* Free the agent expression bytecode as well. We will compute >> - it later on. */ >> - if (loc->cond_bytecode) >> - { >> - free_agent_expr (loc->cond_bytecode); >> - loc->cond_bytecode = NULL; >> - } >> + /* Release the agent expression bytecode as well. We will >> + compute it later on. */ >> + loc->cond_bytecode.reset (); > > Why did you change Free for Release in the comment? Since release has a > different meaning when using unique pointers, it sounds more confusing > like this I think. Hmm, I saw the "Free" and thought that since we now use "delete", release would be a generic term that would be clearer. But you have a good point. Free -> Delete ? Or I can just keep it was Free. Thanks, Pedro Alves