From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59379 invoked by alias); 20 Jul 2017 18:13:33 -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 59365 invoked by uid 89); 20 Jul 2017 18:13:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Thu, 20 Jul 2017 18:13:22 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 74D38267C6; Thu, 20 Jul 2017 18:13:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 74D38267C6 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 74D38267C6 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3CFF26BF6D; Thu, 20 Jul 2017 18:13:21 +0000 (UTC) From: Sergio Durigan Junior To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA v2 1/4] C++-ify break-catch-sig References: <20170719203225.6714-1-tom@tromey.com> <20170719203225.6714-2-tom@tromey.com> Date: Thu, 20 Jul 2017 18:13:00 -0000 In-Reply-To: <20170719203225.6714-2-tom@tromey.com> (Tom Tromey's message of "Wed, 19 Jul 2017 14:32:22 -0600") Message-ID: <87d18vkmdb.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00308.txt.bz2 On Wednesday, July 19 2017, Tom Tromey wrote: > This changes signal_catchpoint to be more of a C++ class, using > std::vector and updating the users. Thanks for the patch, Tom. > 2017-06-04 Tom Tromey > > * break-catch-sig.c (gdb_signal_type): Remove typedef. > (struct signal_catchpoint) : Now a > std::vector. > : Now a bool. > (~signal_catchpoint): Remove. > (signal_catchpoint_insert_location) > (signal_catchpoint_remove_location) > (signal_catchpoint_breakpoint_hit, signal_catchpoint_print_one) > (signal_catchpoint_print_mention) > (signal_catchpoint_print_recreate) > (signal_catchpoint_explains_signal): Update. > (create_signal_catchpoint): Change type of "filter" and > "catch_all". > (catch_signal_split_args): Return a std::vector. Change type of > "catch_all". > (catch_signal_command): Update. > --- > gdb/ChangeLog | 19 +++++++ > gdb/break-catch-sig.c | 143 +++++++++++++++++--------------------------------- > 2 files changed, 66 insertions(+), 96 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index bc6e55b..42bde4b 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,22 @@ > +2017-06-04 Tom Tromey > + > + * break-catch-sig.c (gdb_signal_type): Remove typedef. > + (struct signal_catchpoint) : Now a > + std::vector. > + : Now a bool. > + (~signal_catchpoint): Remove. > + (signal_catchpoint_insert_location) > + (signal_catchpoint_remove_location) > + (signal_catchpoint_breakpoint_hit, signal_catchpoint_print_one) > + (signal_catchpoint_print_mention) > + (signal_catchpoint_print_recreate) > + (signal_catchpoint_explains_signal): Update. > + (create_signal_catchpoint): Change type of "filter" and > + "catch_all". > + (catch_signal_split_args): Return a std::vector. Change type of > + "catch_all". > + (catch_signal_command): Update. > + > 2017-07-18 David Blaikie > > * dwarf2read.c (create_cus_hash_table): Re-add lost initialization > diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c > index 3eede93..afac016 100644 > --- a/gdb/break-catch-sig.c > +++ b/gdb/break-catch-sig.c > @@ -33,30 +33,24 @@ > > #define INTERNAL_SIGNAL(x) ((x) == GDB_SIGNAL_TRAP || (x) == GDB_SIGNAL_INT) > > -typedef enum gdb_signal gdb_signal_type; > - > -DEF_VEC_I (gdb_signal_type); > - > /* An instance of this type is used to represent a signal catchpoint. > A breakpoint is really of this type iff its ops pointer points to > SIGNAL_CATCHPOINT_OPS. */ > > struct signal_catchpoint : public breakpoint > { > - ~signal_catchpoint () override; > - > /* Signal numbers used for the 'catch signal' feature. If no signal > - has been specified for filtering, its value is NULL. Otherwise, > + has been specified for filtering, it is empty. Otherwise, > it holds a list of all signals to be caught. */ > > - VEC (gdb_signal_type) *signals_to_be_caught; > + std::vector signals_to_be_caught; > > - /* If SIGNALS_TO_BE_CAUGHT is NULL, then all "ordinary" signals are > + /* If SIGNALS_TO_BE_CAUGHT is empty, then all "ordinary" signals are > caught. If CATCH_ALL is non-zero, then internal signals are "If CATCH_ALL is true" > - caught as well. If SIGNALS_TO_BE_CAUGHT is non-NULL, then this > + caught as well. If SIGNALS_TO_BE_CAUGHT is not empty, then this > field is ignored. */ > > - int catch_all; > + bool catch_all; > }; > > /* The breakpoint_ops structure to be used in signal catchpoints. */ > @@ -85,13 +79,6 @@ signal_to_name_or_int (enum gdb_signal sig) > > > > -/* signal_catchpoint destructor. */ > - > -signal_catchpoint::~signal_catchpoint () > -{ > - VEC_free (gdb_signal_type, this->signals_to_be_caught); > -} > - > /* Implement the "insert_location" breakpoint_ops method for signal > catchpoints. */ > > @@ -99,20 +86,15 @@ static int > signal_catchpoint_insert_location (struct bp_location *bl) > { > struct signal_catchpoint *c = (struct signal_catchpoint *) bl->owner; > - int i; > > - if (c->signals_to_be_caught != NULL) > + if (!c->signals_to_be_caught.empty ()) > { > - gdb_signal_type iter; > - > - for (i = 0; > - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter); > - i++) > + for (gdb_signal iter : c->signals_to_be_caught) > ++signal_catch_counts[iter]; > } > else > { > - for (i = 0; i < GDB_SIGNAL_LAST; ++i) > + for (int i = 0; i < GDB_SIGNAL_LAST; ++i) > { > if (c->catch_all || !INTERNAL_SIGNAL (i)) > ++signal_catch_counts[i]; > @@ -132,15 +114,10 @@ signal_catchpoint_remove_location (struct bp_location *bl, > enum remove_bp_reason reason) > { > struct signal_catchpoint *c = (struct signal_catchpoint *) bl->owner; > - int i; > > - if (c->signals_to_be_caught != NULL) > + if (!c->signals_to_be_caught.empty ()) > { > - gdb_signal_type iter; > - > - for (i = 0; > - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter); > - i++) > + for (gdb_signal iter : c->signals_to_be_caught) > { > gdb_assert (signal_catch_counts[iter] > 0); > --signal_catch_counts[iter]; > @@ -148,7 +125,7 @@ signal_catchpoint_remove_location (struct bp_location *bl, > } > else > { > - for (i = 0; i < GDB_SIGNAL_LAST; ++i) > + for (int i = 0; i < GDB_SIGNAL_LAST; ++i) > { > if (c->catch_all || !INTERNAL_SIGNAL (i)) > { > @@ -174,7 +151,7 @@ signal_catchpoint_breakpoint_hit (const struct bp_location *bl, > { > const struct signal_catchpoint *c > = (const struct signal_catchpoint *) bl->owner; > - gdb_signal_type signal_number; > + gdb_signal signal_number; > > if (ws->kind != TARGET_WAITKIND_STOPPED) > return 0; > @@ -184,18 +161,12 @@ signal_catchpoint_breakpoint_hit (const struct bp_location *bl, > /* If we are catching specific signals in this breakpoint, then we > must guarantee that the called signal is the same signal we are > catching. */ > - if (c->signals_to_be_caught) > + if (!c->signals_to_be_caught.empty ()) > { > - int i; > - gdb_signal_type iter; > - > - for (i = 0; > - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter); > - i++) > + for (gdb_signal iter : c->signals_to_be_caught) > if (signal_number == iter) > return 1; > /* Not the same. */ > - gdb_assert (!iter); I don't really understand what this assert was checking. In what situation would iter == 0? > return 0; > } > else > @@ -246,26 +217,24 @@ signal_catchpoint_print_one (struct breakpoint *b, > uiout->field_skip ("addr"); > annotate_field (5); > > - if (c->signals_to_be_caught > - && VEC_length (gdb_signal_type, c->signals_to_be_caught) > 1) > + if (!c->signals_to_be_caught.empty ()) Should be: if (c->signals_to_be_caught.size () > 1) > uiout->text ("signals \""); > else > uiout->text ("signal \""); > > - if (c->signals_to_be_caught) > + if (c->signals_to_be_caught.size () > 1) And here: if (!c->signals_to_be_caught.empty ()) > { > - int i; > - gdb_signal_type iter; > std::string text; > > - for (i = 0; > - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter); > - i++) > + bool first = true; > + for (gdb_signal iter : c->signals_to_be_caught) > { > const char *name = signal_to_name_or_int (iter); > > - if (i > 0) > + if (!first) > text += " "; > + first = false; > + > text += name; > } > uiout->field_string ("what", text.c_str ()); > @@ -287,19 +256,14 @@ signal_catchpoint_print_mention (struct breakpoint *b) > { > struct signal_catchpoint *c = (struct signal_catchpoint *) b; > > - if (c->signals_to_be_caught) > + if (!c->signals_to_be_caught.empty ()) > { > - int i; > - gdb_signal_type iter; > - > - if (VEC_length (gdb_signal_type, c->signals_to_be_caught) > 1) > + if (c->signals_to_be_caught.size () > 1) > printf_filtered (_("Catchpoint %d (signals"), b->number); > else > printf_filtered (_("Catchpoint %d (signal"), b->number); > > - for (i = 0; > - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter); > - i++) > + for (gdb_signal iter : c->signals_to_be_caught) > { > const char *name = signal_to_name_or_int (iter); > > @@ -323,14 +287,9 @@ signal_catchpoint_print_recreate (struct breakpoint *b, struct ui_file *fp) > > fprintf_unfiltered (fp, "catch signal"); > > - if (c->signals_to_be_caught) > + if (!c->signals_to_be_caught.empty ()) > { > - int i; > - gdb_signal_type iter; > - > - for (i = 0; > - VEC_iterate (gdb_signal_type, c->signals_to_be_caught, i, iter); > - i++) > + for (gdb_signal iter : c->signals_to_be_caught) > fprintf_unfiltered (fp, " %s", signal_to_name_or_int (iter)); > } > else if (c->catch_all) > @@ -355,8 +314,8 @@ signal_catchpoint_explains_signal (struct breakpoint *b, enum gdb_signal sig) > then internal signals like SIGTRAP are not caught. */ > > static void > -create_signal_catchpoint (int tempflag, VEC (gdb_signal_type) *filter, > - int catch_all) > +create_signal_catchpoint (int tempflag, std::vector &&filter, > + bool catch_all) > { > struct signal_catchpoint *c; > struct gdbarch *gdbarch = get_current_arch (); > @@ -373,57 +332,50 @@ create_signal_catchpoint (int tempflag, VEC (gdb_signal_type) *filter, > /* Splits the argument using space as delimiter. Returns an xmalloc'd > filter list, or NULL if no filtering is required. */ > > -static VEC (gdb_signal_type) * > -catch_signal_split_args (char *arg, int *catch_all) > +static std::vector > +catch_signal_split_args (char *arg, bool *catch_all) > { > - VEC (gdb_signal_type) *result = NULL; > - struct cleanup *cleanup = make_cleanup (VEC_cleanup (gdb_signal_type), > - &result); > + std::vector result; > int first = 1; Nit: "first" can be a bool. > > while (*arg != '\0') > { > int num; > - gdb_signal_type signal_number; > - char *one_arg, *endptr; > - struct cleanup *inner_cleanup; > + gdb_signal signal_number; > + char *endptr; > > - one_arg = extract_arg (&arg); > + gdb::unique_xmalloc_ptr one_arg (extract_arg (&arg)); > if (one_arg == NULL) > break; > - inner_cleanup = make_cleanup (xfree, one_arg); > > /* Check for the special flag "all". */ > - if (strcmp (one_arg, "all") == 0) > + if (strcmp (one_arg.get (), "all") == 0) > { > arg = skip_spaces (arg); > if (*arg != '\0' || !first) > error (_("'all' cannot be caught with other signals")); > - *catch_all = 1; > - gdb_assert (result == NULL); > - do_cleanups (inner_cleanup); > - discard_cleanups (cleanup); > - return NULL; > + *catch_all = true; > + gdb_assert (result.empty ()); > + return result; > } > > first = 0; > > /* Check if the user provided a signal name or a number. */ > - num = (int) strtol (one_arg, &endptr, 0); > + num = (int) strtol (one_arg.get (), &endptr, 0); > if (*endptr == '\0') > signal_number = gdb_signal_from_command (num); > else > { > - signal_number = gdb_signal_from_name (one_arg); > + signal_number = gdb_signal_from_name (one_arg.get ()); > if (signal_number == GDB_SIGNAL_UNKNOWN) > - error (_("Unknown signal name '%s'."), one_arg); > + error (_("Unknown signal name '%s'."), one_arg.get ()); > } > > - VEC_safe_push (gdb_signal_type, result, signal_number); > - do_cleanups (inner_cleanup); > + result.push_back (signal_number); > } > > - discard_cleanups (cleanup); > + result.shrink_to_fit (); Do you really need this call here? You're using safe_push above, so I don't think the vector will end up with a bigger capacity than its size. > return result; > } > > @@ -433,8 +385,9 @@ static void > catch_signal_command (char *arg, int from_tty, > struct cmd_list_element *command) > { > - int tempflag, catch_all = 0; > - VEC (gdb_signal_type) *filter; > + int tempflag; > + bool catch_all = false; > + std::vector filter; > > tempflag = get_cmd_context (command) == CATCH_TEMPORARY; > > @@ -448,10 +401,8 @@ catch_signal_command (char *arg, int from_tty, > > if (arg != NULL) > filter = catch_signal_split_args (arg, &catch_all); > - else > - filter = NULL; > > - create_signal_catchpoint (tempflag, filter, catch_all); > + create_signal_catchpoint (tempflag, std::move (filter), catch_all); > } > > static void > -- > 2.9.3 Otherwise, LGTM. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/