From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 4094A3858D3C for ; Tue, 12 Sep 2023 09:15:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4094A3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1694510119; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=QdLhWGlI8AN4p5bq29n9g0U0ZBhsuPEm1xOOlXWevZo=; b=UsM22X7WOiQpTs7UdMRlzFEDuNkQ7m+8/QO/HRB+itmPIcfVsw3kcJ/I8TTiHMukvMjZGG g7Zw9DpXTOTVyUcPw4JhNMPvInBF/3q3qobOH0pXfaDaOrlXe1/TcZhk2dNHUZ5XAKiTGp P4CxzGIARpp/akH1vWGWBo+ZQI96oJo= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-13-XAJ8L9t6NjS5wn3rYLLZ_A-1; Tue, 12 Sep 2023 05:15:18 -0400 X-MC-Unique: XAJ8L9t6NjS5wn3rYLLZ_A-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-402cd372b8bso43166715e9.2 for ; Tue, 12 Sep 2023 02:15:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694510117; x=1695114917; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=QdLhWGlI8AN4p5bq29n9g0U0ZBhsuPEm1xOOlXWevZo=; b=FuzLHXy8i9XQmGUVYj4DEtr9lGYyhNaec4HnsShlBiUjysWNtRubMxI4j6oDaVCqTP bkhBhJOXxjvfAZhQtM7ft3kiKJizeSh57uF+pUNoqxXP9MqCEeFOgxt6p+zAOV99W00c gzvppR1fpjleu+Tpjks1yNDvv5GrubGjbmXQ3v/QINSZP7ViRy2ICBW1ytiBFqSA2e0U cGwk/EUlBmi11M0VF6noTlGxpjokZdxnsb65/lOI+m6XJND9+oz/oF0CRZJ2R7InoPlw H+gAQh8OIf55UR/wSrZMNOuzPymYljeUd6xFGiwXBKKV0WVSTexl1C7pGZVLehXJ7Uhj gZ9g== X-Gm-Message-State: AOJu0Yzo57tr9G2QdoLzQL+DiNIostjlMYEb/uDYKmtgxq8sGiIdHrpi AWsm243lE9gqg56v81NGymmhbahkEsjXG9KxzuzmGkxbrHba343118RXtbEROjbliA+B7jDYdPS RFaOs/64QgFMxNQLg7Ykh4NNgsL7nqDAIAGkNaa5buqs8fHsnFVcASetXEJyM4NM+1eiifuUAgj uLL0z+jA== X-Received: by 2002:a7b:c8c4:0:b0:402:8c7e:3fc4 with SMTP id f4-20020a7bc8c4000000b004028c7e3fc4mr9786603wml.30.1694510117237; Tue, 12 Sep 2023 02:15:17 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHb7cLeiYrEiV6OT8T1AUkP+q8TwBkQ9jN+RDSWiAhqcE9nvDIMePWB5JLgb70gWYik+yY2xw== X-Received: by 2002:a7b:c8c4:0:b0:402:8c7e:3fc4 with SMTP id f4-20020a7bc8c4000000b004028c7e3fc4mr9786582wml.30.1694510116644; Tue, 12 Sep 2023 02:15:16 -0700 (PDT) Received: from localhost (197.126.90.146.dyn.plus.net. [146.90.126.197]) by smtp.gmail.com with ESMTPSA id 18-20020a05600c249200b003fe1a96845bsm15608697wms.2.2023.09.12.02.15.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Sep 2023 02:15:16 -0700 (PDT) From: Andrew Burgess To: Simon Marchi via Gdb-patches , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH 04/21] gdb: make interp_lookup a method of struct ui In-Reply-To: <20230908190227.96319-5-simon.marchi@efficios.com> References: <20230908190227.96319-1-simon.marchi@efficios.com> <20230908190227.96319-5-simon.marchi@efficios.com> Date: Tue, 12 Sep 2023 10:15:14 +0100 Message-ID: <87ledbkb19.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Simon Marchi via Gdb-patches writes: > This requires exposing some things about interpreter factories in > interps.h, for ui.c to use it. No behavior changes expected. > > Change-Id: I7a9e93621c0588e367b5356916d4dad90757bb3d > --- > gdb/cli/cli-script.c | 2 +- > gdb/interps.c | 58 ++++++++++++-------------------------------- > gdb/interps.h | 25 ++++++++++++++----- > gdb/mi/mi-interp.c | 4 +-- > gdb/python/python.c | 2 +- > gdb/ui.c | 25 ++++++++++++++++++- > gdb/ui.h | 6 +++++ > 7 files changed, 68 insertions(+), 54 deletions(-) > > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c > index 8ec5689ebcfd..49ba854c2a09 100644 > --- a/gdb/cli/cli-script.c > +++ b/gdb/cli/cli-script.c > @@ -702,7 +702,7 @@ execute_control_command (struct command_line *cmd, int from_tty) > > /* Make sure we use the console uiout. It's possible that we are executing > breakpoint commands while running the MI interpreter. */ > - interp *console = interp_lookup (current_ui, INTERP_CONSOLE); > + interp *console = current_ui->lookup_interp (INTERP_CONSOLE); > scoped_restore save_uiout > = make_scoped_restore (¤t_uiout, console->interp_ui_out ()); > > diff --git a/gdb/interps.c b/gdb/interps.c > index b05a6c4eb875..bd95c3175adb 100644 > --- a/gdb/interps.c > +++ b/gdb/interps.c > @@ -48,22 +48,6 @@ interp::interp (const char *name) > > interp::~interp () = default; > > -/* An interpreter factory. Maps an interpreter name to the factory > - function that instantiates an interpreter by that name. */ > - > -struct interp_factory > -{ > - interp_factory (const char *name_, interp_factory_func func_) > - : name (name_), func (func_) > - {} > - > - /* This is the name in "-i=INTERP" and "interpreter-exec INTERP". */ > - const char *name; > - > - /* The function that creates the interpreter. */ > - interp_factory_func func; > -}; > - > /* The registered interpreter factories. */ > static std::vector interpreter_factories; > > @@ -83,6 +67,18 @@ interp_factory_register (const char *name, interp_factory_func func) > interpreter_factories.emplace_back (name, func); > } > > +/* See interps.h. */ > + > +const interp_factory * > +find_interp_factory (const char *name) > +{ > + for (const interp_factory &factory : interpreter_factories) > + if (strcmp (factory.name, name) == 0) > + return &factory; > + > + return nullptr; > +} > + > /* This sets the current interpreter to be INTERP. If INTERP has not > been initialized, then this will also run the init method. > > @@ -145,35 +141,11 @@ interp_set (struct interp *interp, bool top_level) > > /* See interps.h. */ > > -struct interp * > -interp_lookup (struct ui *ui, const char *name) > -{ > - if (name == NULL || strlen (name) == 0) > - return NULL; > - > - /* Only create each interpreter once per top level. */ > - interp *interp = ui->lookup_existing_interp (name); > - if (interp != NULL) > - return interp; > - > - for (const interp_factory &factory : interpreter_factories) > - if (strcmp (factory.name, name) == 0) > - { > - interp = factory.func (factory.name); > - ui->add_interp (interp); > - return interp; > - } > - > - return NULL; > -} > - > -/* See interps.h. */ > - > void > set_top_level_interpreter (const char *name) > { > /* Find it. */ > - struct interp *interp = interp_lookup (current_ui, name); > + struct interp *interp = current_ui->lookup_interp (name); > > if (interp == NULL) > error (_("Interpreter `%s' unrecognized"), name); > @@ -194,7 +166,7 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect, > struct interp * > scoped_restore_interp::set_interp (const char *name) > { > - struct interp *interp = interp_lookup (current_ui, name); > + struct interp *interp = current_ui->lookup_interp (name); > struct interp *old_interp = current_ui->current_interpreter; > > if (interp) > @@ -290,7 +262,7 @@ interpreter_exec_cmd (const char *args, int from_tty) > > interp *old_interp = current_ui->current_interpreter; > > - interp_to_use = interp_lookup (current_ui, prules[0]); > + interp_to_use = current_ui->lookup_interp (prules[0]); > if (interp_to_use == NULL) > error (_("Could not find interpreter \"%s\"."), prules[0]); > > diff --git a/gdb/interps.h b/gdb/interps.h > index 287df2c8c810..433d92439eba 100644 > --- a/gdb/interps.h > +++ b/gdb/interps.h > @@ -36,6 +36,22 @@ struct trace_state_variable; > > typedef struct interp *(*interp_factory_func) (const char *name); > > +/* An interpreter factory. Maps an interpreter name to the factory > + function that instantiates an interpreter by that name. */ > + > +struct interp_factory > +{ > + interp_factory (const char *name_, interp_factory_func func_) > + : name (name_), func (func_) > + {} > + > + /* This is the name in "-i=INTERP" and "interpreter-exec INTERP". */ > + const char *name; > + > + /* The function that creates the interpreter. */ > + interp_factory_func func; > +}; > + > /* Each interpreter kind (CLI, MI, etc.) registers itself with a call > to this function, passing along its name, and a pointer to a > function that creates a new instance of an interpreter with that > @@ -45,6 +61,9 @@ typedef struct interp *(*interp_factory_func) (const char *name); > extern void interp_factory_register (const char *name, > interp_factory_func func); > > +/* Return the interpreter factory for NAME. Return nullptr if not found. */ > +extern const interp_factory *find_interp_factory (const char *name); > + > extern void interp_exec (struct interp *interp, const char *command); > > class interp : public intrusive_list_node > @@ -193,12 +212,6 @@ class interp : public intrusive_list_node > bool inited = false; > }; > > -/* Look up the interpreter for NAME, creating one if none exists yet. > - If NAME is not a interpreter type previously registered with > - interp_factory_register, return NULL; otherwise return a pointer to > - the interpreter. */ > -extern struct interp *interp_lookup (struct ui *ui, const char *name); > - > /* Set the current UI's top level interpreter to the interpreter named > NAME. Throws an error if NAME is not a known interpreter or the > interpreter fails to initialize. */ > diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c > index b3b0f5bb1f51..946fed5867c4 100644 > --- a/gdb/mi/mi-interp.c > +++ b/gdb/mi/mi-interp.c > @@ -162,7 +162,7 @@ mi_cmd_interpreter_exec (const char *command, const char *const *argv, > error (_("-interpreter-exec: " > "Usage: -interpreter-exec interp command")); > > - interp_to_use = interp_lookup (current_ui, argv[0]); > + interp_to_use = current_ui->lookup_interp (argv[0]); > if (interp_to_use == NULL) > error (_("-interpreter-exec: could not find interpreter \"%s\""), > argv[0]); > @@ -416,7 +416,7 @@ mi_interp::on_normal_stop (struct bpstat *bs, int print_frame) > mi_uiout->field_string ("reason", async_reason_lookup (reason)); > } > > - interp *console_interp = interp_lookup (current_ui, INTERP_CONSOLE); > + interp *console_interp = current_ui->lookup_interp (INTERP_CONSOLE); > > /* We only want to print the displays once, and we want it to > look just how it would on the console, so we use this to > diff --git a/gdb/python/python.c b/gdb/python/python.c > index 6a978d632e9a..a1aaa29d0ce5 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -683,7 +683,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw) > > /* Use the console interpreter uiout to have the same print format > for console or MI. */ > - interp = interp_lookup (current_ui, "console"); > + interp = current_ui->lookup_interp ("console"); Might as well take this opportunity to replace "console" with INTERP_CONSOLE which is what we use everywhere else. > current_uiout = interp->interp_ui_out (); > > if (to_string) > diff --git a/gdb/ui.c b/gdb/ui.c > index 624187ac73c3..a7b81c077e9a 100644 > --- a/gdb/ui.c > +++ b/gdb/ui.c > @@ -169,7 +169,30 @@ ui::add_interp (interp *interp) > this->interp_list.push_back (*interp); > } > > -/* See top.h. */ > +/* See interps.h. */ > + > +interp * > +ui::lookup_interp (const char *name) > +{ > + if (name == nullptr || strlen (name) == 0) > + return nullptr; > + > + /* Only create each interpreter once per UI. */ > + interp *interp = this->lookup_existing_interp (name); > + if (interp != nullptr) > + return interp; > + > + const interp_factory *factory = find_interp_factory (name); > + if (factory == nullptr) > + return nullptr; > + > + interp = factory->func (factory->name); > + this->add_interp (interp); > + > + return interp; > +} Rather than exposing the interpreter factor stuff outside of interp.c, I think a better approach would be either move ui::lookup_interp into interp.c -- personally, I'm happy to see different parts of an object in different .c files when that makes sense. But if you prefer all to keep ui implementation in ui.c, rather than calling find_interp_factory and then factory->func, we could just have a new function `create_interp (name)` which creates an interpreter, or returns nullptr, then we can write: interp = create_interp (name); if (interp != nullptr) this->add_interp (interp); return interp; And all the factory stuff can remain private to interp.c. Thoughts? Thanks, Andrew > + > +/* See ui.h. */ > > void > ui::unregister_file_handler () > diff --git a/gdb/ui.h b/gdb/ui.h > index 48cad96b3fb8..aeb26c68823a 100644 > --- a/gdb/ui.h > +++ b/gdb/ui.h > @@ -163,6 +163,12 @@ struct ui : public intrusive_list_node > return nullptr, otherwise return a pointer to the interpreter. */ > interp *lookup_existing_interp (const char *name) const; > > + /* Look up the interpreter for NAME, creating one if none exists yet. > + If NAME is not a interpreter type previously registered with > + interp_factory_register, return nullptr; otherwise return a pointer to > + the interpreter. */ > + interp *lookup_interp (const char *name); > + > /* Add interpreter INTERP to this UI's interpreter list. The > interpreter must not have previously been added. */ > void add_interp (interp *interp); > -- > 2.42.0