From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id CB0393857374 for ; Fri, 20 May 2022 13:41:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CB0393857374 Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 6D20686E09; Fri, 20 May 2022 13:41:02 +0000 (UTC) Date: Fri, 20 May 2022 13:40:57 +0000 From: Lancelot SIX To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint Message-ID: <20220520133941.lymcglw2sovyjycn@ubuntu.lan> References: <20220516184030.665489-1-pedro@palves.net> <20220516184030.665489-10-pedro@palves.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220516184030.665489-10-pedro@palves.net> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Fri, 20 May 2022 13:41:02 +0000 (UTC) X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 May 2022 13:41:06 -0000 Hi, Some styling remarks below. On Mon, May 16, 2022 at 07:40:16PM +0100, Pedro Alves wrote: > This converts init_breakpoint_sal to a base_breakpoint constructor. > > It removes a use of init_raw_breakpoint. > > To avoid manually adding a bunch of parameters to > new_breakpoint_from_type, and manually passing them down to the > constructors of a number of different base_breakpoint subclasses, make > new_breakpoint_from_type a variable template function. > > Change-Id: I4cc24133ac4c292f547289ec782fc78e5bbe2510 > --- > gdb/breakpoint.c | 239 +++++++++++++++++++++++------------------------ > gdb/breakpoint.h | 16 ++++ > 2 files changed, 133 insertions(+), 122 deletions(-) > @@ -8326,81 +8331,68 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch, > > gdb_assert (!sals.empty ()); > > - for (const auto &sal : sals) > - { > - struct bp_location *loc; > + thread = thread_; > + task = task_; > > - if (from_tty) > - { > - struct gdbarch *loc_gdbarch = get_sal_arch (sal); > - if (!loc_gdbarch) > - loc_gdbarch = gdbarch; > + cond_string = std::move (cond_string_); > + extra_string = std::move (extra_string_); > + ignore_count = ignore_count_; > + enable_state = enabled_ ? bp_enabled : bp_disabled; > + disposition = disposition_; > > - describe_other_breakpoints (loc_gdbarch, > - sal.pspace, sal.pc, sal.section, thread); > - } > + if (type == bp_static_tracepoint > + || type == bp_static_marker_tracepoint) > + { > + auto *t = static_cast (this); I think the space before "<" should be removed (I think I saw this in one of the previous patches too). > + struct static_tracepoint_marker marker; > > - if (&sal == &sals[0]) > + if (strace_marker_p (this)) > { > - init_raw_breakpoint (b, sal, type); > - b->thread = thread; > - b->task = task; > + /* We already know the marker exists, otherwise, we > + wouldn't see a sal for it. */ > + const char *p > + = &event_location_to_string (location_.get ())[3]; Looks like that the statement can fit on one line now. > + const char *endp; > > - b->cond_string = std::move (cond_string); > - b->extra_string = std::move (extra_string); > - b->ignore_count = ignore_count; > - b->enable_state = enabled ? bp_enabled : bp_disabled; > - b->disposition = disposition; > + p = skip_spaces (p); > > - if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0) > - b->loc->inserted = 1; > + endp = skip_to_space (p); > > - if (type == bp_static_tracepoint > - || type == bp_static_marker_tracepoint) > - { > - auto *t = static_cast (b); > - struct static_tracepoint_marker marker; > - > - if (strace_marker_p (b)) > - { > - /* We already know the marker exists, otherwise, we > - wouldn't see a sal for it. */ > - const char *p > - = &event_location_to_string (b->location.get ())[3]; > - const char *endp; > - > - p = skip_spaces (p); > - > - endp = skip_to_space (p); > - > - t->static_trace_marker_id.assign (p, endp - p); > - > - gdb_printf (_("Probed static tracepoint " > - "marker \"%s\"\n"), > - t->static_trace_marker_id.c_str ()); > - } > - else if (target_static_tracepoint_marker_at (sal.pc, &marker)) > - { > - t->static_trace_marker_id = std::move (marker.str_id); > + t->static_trace_marker_id.assign (p, endp - p); > > - gdb_printf (_("Probed static tracepoint " > - "marker \"%s\"\n"), > - t->static_trace_marker_id.c_str ()); > - } > - else > - warning (_("Couldn't determine the static " > - "tracepoint marker to probe")); > - } > + gdb_printf (_("Probed static tracepoint " > + "marker \"%s\"\n"), This literal string can also fit into one line, no need to split it. > + t->static_trace_marker_id.c_str ()); > + } > + else if (target_static_tracepoint_marker_at (sals[0].pc, &marker)) > + { > + t->static_trace_marker_id = std::move (marker.str_id); > > - loc = b->loc; > + gdb_printf (_("Probed static tracepoint " > + "marker \"%s\"\n"), Same here. > + t->static_trace_marker_id.c_str ()); > } > else > + warning (_("Couldn't determine the static " > + "tracepoint marker to probe")); And probably here too. > + } > + > + for (const auto &sal : sals) > + { > + if (from_tty) > { > - loc = b->add_location (sal); > - if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0) > - loc->inserted = 1; > + struct gdbarch *loc_gdbarch = get_sal_arch (sal); > + if (!loc_gdbarch) Should be "loc_gdbarch == nullptr" > + loc_gdbarch = gdbarch; > + > + describe_other_breakpoints (loc_gdbarch, > + sal.pspace, sal.pc, sal.section, thread); > } > > + bp_location *new_loc = add_location (sal); > + if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0) > + new_loc->inserted = 1; > + > /* Do not set breakpoint locations conditions yet. As locations > are inserted, they get sorted based on their addresses. Let > the list stabilize to have reliable location numbers. */ Also, down the line, there are some leftover NULL which could become nullptr. This is from code which just god re-indented, but while at updating it, it could be nice to fix this too. Best, Lancelot.