Re: [PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint

[Lancelot SIX <lsix@lancelotsix.com>]

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 <struct tracepoint *> (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 <struct tracepoint *> (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.