Re: [PATCH v8 1/3] MI: add the -catch-load and -catch-unload commands

[Mircea Gherzan <mircea dot gherzan at intel dot com>]

On 07.12.2012 18:31, Tom Tromey wrote:
> > > > > > "Mircea" == Mircea Gherzan <mircea.gherzan@intel.com> writes:
>
> Mircea> They are equivalent to "catch load" and "catch unload" from CLI.
> Mircea> Rationale: GUIs might be interested in catching solib load or
> Mircea> unload events.
>
> Thanks.
>
> Mircea> -static int mi_can_breakpoint_notify;
> Mircea> +int mi_can_breakpoint_notify;
>
> I don't think this change is needed.
>
> Mircea>  static void
> Mircea>  breakpoint_notify (struct breakpoint *b)
> Mircea>  {
> Mircea> -  if (mi_can_breakpoint_notify)
> Mircea> +  int can_notify = (b->type == bp_catchpoint
> Mircea> +                    ? mi_can_catchpoint_notify
> Mircea> +                    : mi_can_breakpoint_notify);
> Mircea> +
> Mircea> +  if (can_notify)
> Mircea>      gdb_breakpoint_query (current_uiout, b->number, NULL);
>
> You might as well just inline the condition instead of introducing a new
> variable.
>
> Why have two globals here instead of just one?
>
> Mircea> +/* Setup the reporting of the insertion of a new breakpoint
> Mircea> +   or catchpoint.  The flag at CAN_NOTIFY controls whether the
> Mircea> +   notifier may act.  The REV_FLAG is used to return a cleanup
> Mircea> +   for the initial value of CAN_NOTIFY.  */
> Mircea> +
> Mircea> +void setup_breakpoint_reporting (int *can_notify,
> Mircea> +                                 struct cleanup **rev_flag)
>
> Newline after 'void'.
>
> It is more typical for a function like this to return the new cleanup.
>
> If you had just a single global you could remove the 'can_notify'
> argument.
>
> Mircea> +/* True if MI catchpoint observers have been registered.  */
> Mircea> +
> Mircea> +static int mi_catchpoint_observers_installed;
>
> This is no longer used.
>
> Mircea> +  setup_breakpoint_reporting(&mi_can_catchpoint_notify,
> Mircea> +                             &back_to);
>
> Space before open paren.

Thanks for your review. The issues have been addressed in v9 of the series.

Regards,
Mircea

--

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052