From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107449 invoked by alias); 22 Nov 2017 22:36:11 -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 107438 invoked by uid 89); 22 Nov 2017 22:36:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=sk:inter-d, sk:interd, probe_ops 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, 22 Nov 2017 22:36:09 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 48250C9D2C; Wed, 22 Nov 2017 22:36:08 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0A6496266D; Wed, 22 Nov 2017 22:36:07 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches , Simon Marchi Subject: Re: [PATCH v2 1/3] Convert generic probe interface to C++ (and perform some cleanups) References: <20171113175901.25367-1-sergiodj@redhat.com> <20171116043734.10137-1-sergiodj@redhat.com> <20171116043734.10137-2-sergiodj@redhat.com> Date: Wed, 22 Nov 2017 22:36:00 -0000 In-Reply-To: (Simon Marchi's message of "Wed, 22 Nov 2017 15:39:15 -0500") Message-ID: <87d14adjqg.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg00521.txt.bz2 On Wednesday, November 22 2017, Simon Marchi wrote: > On 2017-11-15 11:37 PM, Sergio Durigan Junior wrote: >> Changes from v1: >> >> - Remove emit_info_probes_extra_fields method. >> >> - Make gen_info_* methods return the vector directly. Modify code >> accordingly. >> >> - Cleanup unused variables. >> >> - Make can_evaluate_arguments return bool. >> >> - Make get_name and get_provider return a const std::string &. Modify >> code accordingly. >> >> - Move can_enable method from 'class probe' to 'class >> static_probe_ops'. >> >> - Use const references when iterating on vector of objects. >> >> This patch converts the generic probe interface (gdb/probe.[ch]) to >> C++, and also performs some cleanups that were on my TODO list for a >> while. >> >> The main changes were the conversion of 'struct probe' to 'class >> probe', and 'struct probe_ops' to 'class static_probe_ops'. The >> former now contains all the "dynamic", generic methods that act on a >> probe + the generic data related to it; the latter encapsulates a >> bunch of "static" methods that relate to the probe type, but not to a >> specific probe itself. >> >> I've had to do a few renamings (e.g., on 'struct bound_probe' the >> field is called 'probe *prob' now, instead of 'struct probe *probe') >> because GCC was complaining about naming the field using the same name >> as the class. Nothing major, though. Generally speaking, the logic >> behind and the design behind the code are the same. >> >> Even though I'm sending a series of patches, they need to be tested >> and committed as a single unit, because of inter-dependencies. But it >> should be easier to review in separate logical units. >> >> I've regtested this patch on BuildBot, no regressions found. > > Hi Sergio, Hey Simon, Thanks for the review. > I've looked at it (the series) quickly, and it looks good to me. Personally, > for comparing std::string with char*, I would use the form > > str != char_ptr > > rather than > > str.compare (char_ptr) != 0 > > since I find it more readable, but I don't really mind. You're right, it's better. I'll replace that in my code. > One whitespace comment below. > >> @@ -334,56 +358,33 @@ compare_probes (const bound_probe &a, const bound_probe &b) >> >> static void >> gen_ui_out_table_header_info (const std::vector &probes, >> - const struct probe_ops *p) >> + const static_probe_ops *spops) >> { >> /* `headings' refers to the names of the columns when printing `info >> probes'. */ >> - VEC (info_probe_column_s) *headings = NULL; >> - struct cleanup *c; >> - info_probe_column_s *column; >> - size_t headings_size; >> - int ix; >> + gdb_assert (spops != NULL); >> >> - gdb_assert (p != NULL); >> + std::vector headings >> + = spops->gen_info_probes_table_header (); >> >> - if (p->gen_info_probes_table_header == NULL >> - && p->gen_info_probes_table_values == NULL) >> - return; >> - >> - gdb_assert (p->gen_info_probes_table_header != NULL >> - && p->gen_info_probes_table_values != NULL); >> - >> - c = make_cleanup (VEC_cleanup (info_probe_column_s), &headings); >> - p->gen_info_probes_table_header (&headings); >> - >> - headings_size = VEC_length (info_probe_column_s, headings); >> - >> - for (ix = 0; >> - VEC_iterate (info_probe_column_s, headings, ix, column); >> - ++ix) >> + for (const struct info_probe_column &column : headings) >> { >> - size_t size_max = strlen (column->print_name); >> + size_t size_max = strlen (column.print_name); >> >> for (const bound_probe &probe : probes) >> { >> /* `probe_fields' refers to the values of each new field that this >> probe will display. */ >> - VEC (const_char_ptr) *probe_fields = NULL; >> - struct cleanup *c2; >> - const char *val; >> - int kx; >> >> - if (probe.probe->pops != p) >> + if (probe.prob->get_static_ops () != spops) >> continue; >> >> - c2 = make_cleanup (VEC_cleanup (const_char_ptr), &probe_fields); >> - p->gen_info_probes_table_values (probe.probe, &probe_fields); >> + std::vector probe_fields > > There are spaces before the tab in this line. Thanks for catching that. Fixed. Is it OK to push after these changes, or do you want me to submit a v2? 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/