From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27328 invoked by alias); 5 Mar 2018 22:16:27 -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 27302 invoked by uid 89); 5 Mar 2018 22:16:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 05 Mar 2018 22:16:25 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w25MGJfG007177 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 5 Mar 2018 17:16:23 -0500 Received: by simark.ca (Postfix, from userid 112) id 293F61E66B; Mon, 5 Mar 2018 17:16:19 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id ABCF11E584; Mon, 5 Mar 2018 17:16:16 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 05 Mar 2018 22:16:00 -0000 From: Simon Marchi To: "Metzger, Markus T" Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/3] btrace: Remove ui_out cleanups In-Reply-To: References: <20180304205605.13037-1-simon.marchi@polymtl.ca> <20180304205605.13037-3-simon.marchi@polymtl.ca> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.4 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 5 Mar 2018 22:16:19 +0000 X-IsSubscribed: yes X-SW-Source: 2018-03/txt/msg00126.txt.bz2 On 2018-03-05 07:39, Metzger, Markus T wrote: > Hello Simon, > > >> This patch replaces the cleanups that close the list and tuple of the >> btrace instruction history output with ui_out_emit_tuple and >> ui_out_emit_list. >> >> This allows removing make_cleanup_ui_out_tuple_begin_end and >> make_cleanup_ui_out_list_begin_end. >> >> This patch (along with the previous ones in the series) was regtested >> on >> the buildbot. > > >> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c >> index 15ce760f5a..ddd15c4781 100644 >> --- a/gdb/record-btrace.c >> +++ b/gdb/record-btrace.c >> @@ -620,26 +620,25 @@ btrace_find_line_range (CORE_ADDR pc) >> >> static void >> btrace_print_lines (struct btrace_line_range lines, struct ui_out >> *uiout, >> - struct cleanup **ui_item_chain, int flags) >> + gdb::optional *src_and_asm_tuple, >> + gdb::optional *asm_list, > > Reference instead of pointer? I once pointed this out on one of Tom's patches, and he said that in the caller code, it's more obvious that object is meant to be modified if you do: function_that_modifies_object (&object); instead of function_that_modifies_object (object); And I kind of agree with that it is, which is why I've been using pointers when references would have worked (ok, here the function name makes it obvious but it's not always the case). In the implementation of the function, since you use object->field instead of object.field, it also hints that you're not modifying a local object. But I also agree that it's really not C++-y to do it this way, so I'll happily change it. >> - for (line = lines.begin; line < lines.end; ++line) >> + for (int line = lines.begin; line < lines.end; ++line) >> { >> - if (*ui_item_chain != NULL) >> - do_cleanups (*ui_item_chain); >> + asm_list->reset (); >> + src_and_asm_tuple->reset (); > > The SRC_AND_ASM_TUPLE reset () shouldn't be necessary; it is reset () > in emplace (). Ok. >> - *ui_item_chain >> - = make_cleanup_ui_out_tuple_begin_end (uiout, "src_and_asm_line"); >> + src_and_asm_tuple->emplace (uiout, "src_and_asm_line"); >> >> print_source_lines (lines.symtab, line, line + 1, psl_flags); >> >> - make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn"); >> + asm_list->emplace (uiout, "line_asm_insn"); >> } >> } > > Looks good to me, otherwise. > > I have not tested it, though. Did you run the gdb.btrace test suite? > You said > you ran buildbot but that might skip gdb.btrace when run on VMs or on > old > hardware. > > Please let me know if you want me to run the gdb.btrace tests for you. > A user > branch would be nice in that case. Yes, I have ran the gdb.btrace/*.exp tests locally on two different machines and saw no regressions. However, the processors may be a bit old (Q6600 from 2007 and i5-4310U from 2014), so it's possible that not all required features are available, and therefore some tests may be skipped. So if you want to be sure, here's a branch for you to test: users/simark/btrace-cleanups https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/btrace-cleanups Thanks! Simon