From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id WyHsJVHx1F8nEQAAWB0awg (envelope-from ) for ; Sat, 12 Dec 2020 11:35:29 -0500 Received: by simark.ca (Postfix, from userid 112) id 8F0291F0AA; Sat, 12 Dec 2020 11:35:29 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E0AB11EF4B for ; Sat, 12 Dec 2020 11:35:28 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 66212389246C; Sat, 12 Dec 2020 16:35:28 +0000 (GMT) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id B9E53389246C for ; Sat, 12 Dec 2020 16:35:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B9E53389246C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id A905CADB3; Sat, 12 Dec 2020 16:35:24 +0000 (UTC) To: Tom Tromey References: <6ad82f30-0ca3-8b19-2907-d71f0dfecafe@suse.cz> <2c28a403-a169-0dc3-9b64-58484237c8c2@suse.cz> <72061fbb-b480-04ce-ef4a-fe13a7266885@suse.de> <87a6ulmppr.fsf@tromey.com> <5506b198-c504-0046-0559-bcc24538d31c@suse.de> <87k0togndu.fsf@tromey.com> From: Tom de Vries Subject: Re: [gdb/cli] Add a progress meter Message-ID: <2673dab0-5e37-e7dc-a6f8-bb764aa22d6a@suse.de> Date: Sat, 12 Dec 2020 17:35:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <87k0togndu.fsf@tromey.com> Content-Type: multipart/mixed; boundary="------------5D0BC89E2EF9D9C878EDD374" Content-Language: en-US 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: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" This is a multi-part message in MIME format. --------------5D0BC89E2EF9D9C878EDD374 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 12/11/20 8:21 PM, Tom Tromey wrote: >>>>>> "Tom" == Tom de Vries writes: > > Tom> I've taken a look, and think that this code is more in line with gdb cli > Tom> setup, so I've reimplemented on top of this. > > Tom> Here is the first patch, adding the progress meter infrastructure > Tom> factored out from your patch. > > Not sure if I ought to review a patch that I largely wrote, but I did > find some things I think should be done differently nowadays. > > Tom> +void > Tom> +cli_ui_out::do_progress_start (const std::string &name, int should_print) > > should_print should probably be a bool, throughout this patch. > Done. > Tom> + /* Don't actually emit anything until the first call notify us > > I think s/notify/notifies/ > Done. > Tom> + if (!stream->isatty ()) > Tom> + { > Tom> + fprintf_unfiltered (stream, "%s...", meter.name.c_str ()); > > This is probably fine but I suppose the meaning of "should_print" ought > to be described somewhere, Done. > since otherwise this could seem like a bug -- > it is printing something even when should_print==false. > > Tom> + m_meters.push_back (meter); > > std::move(meter) would be more efficient here, since it would avoid > another string copy. > Done. > Tom> + progress_meter (struct ui_out *uiout, const std::string &name, > Tom> + int should_print) > Tom> + : m_uiout (uiout) > > Here's probably where should_print should be documented. > > Tom> +get_chars_per_line (void) > > Just "()", not "(void)", now. > > Tom> +++ b/gdb/utils.h > Tom> @@ -364,6 +364,10 @@ extern void wrap_here (const char *); > > Tom> extern void reinitialize_more_filter (void); > > Tom> +/* Return the number of characters in a line. */ > Tom> + > Tom> +extern int get_chars_per_line (void); > > Here too. Done. I've also fixed the first-progress-call-has-howmuch=1.0 problem mentioned in the original review conversation with Pedro, by adding a new enum meter_state element PROGRESS. I've also fixed the problem that when f.i. run with -batch, chars_per_line is zero. The previous version of the patch was printing "[]" in that case. Thanks, - Tom --------------5D0BC89E2EF9D9C878EDD374 Content-Type: text/x-patch; charset=UTF-8; name="0010-gdb-cli-Add-a-progress-meter.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline; filename="0010-gdb-cli-Add-a-progress-meter.patch" [gdb/cli] Add a progress meter Add a progress meter. It's not used anywhere yet. gdb/ChangeLog: 2020-12-11 Tom Tromey Tom de Vries * utils.h (get_chars_per_line): Declare. * utils.c (get_chars_per_line): New function. (fputs_maybe_filtered): Handle '\r'. * ui-out.h (ui_out::progress_meter): New class. (ui_out::progress, ui_out::do_progress_start) (ui_out::do_progress_notify, ui_out::do_progress_end): New methods. * ui-out.c (do_progress_end) (make_cleanup_ui_out_progress_begin_end, ui_out_progress): New functions. * mi/mi-out.h (mi_ui_out::do_progress_start) (mi_ui_out::do_progress_notify, mi_ui_out::do_progress_end): New methods. * cli-out.h (struct cli_ui_out) : New methods. : New. : New member. * cli-out.c (cli_ui_out::do_progress_start) (cli_ui_out::do_progress_notify, cli_ui_out::do_progress_end): New methods. --- gdb/cli-out.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++ gdb/cli-out.h | 31 +++++++++++++++++ gdb/mi/mi-out.h | 12 +++++++ gdb/ui-out.h | 37 +++++++++++++++++++++ gdb/utils.c | 14 ++++++++ gdb/utils.h | 4 +++ 6 files changed, 199 insertions(+) diff --git a/gdb/cli-out.c b/gdb/cli-out.c index e47272ad87..7722ecc4fe 100644 --- a/gdb/cli-out.c +++ b/gdb/cli-out.c @@ -265,6 +265,107 @@ cli_ui_out::do_redirect (ui_file *outstream) m_streams.pop_back (); } =20 +/* The cli_ui_out::do_progress_* functions result in the following: + - printed for tty, SHOULD_PRINT =3D=3D true: + + - printed for tty, SHOULD_PRINT =3D=3D false: + <> + - printed for not-a-tty: + +*/ + +void +cli_ui_out::do_progress_start (const std::string &name, bool should_prin= t) +{ + struct ui_file *stream =3D m_streams.back (); + cli_progress_info meter; + + meter.last_value =3D 0; + meter.name =3D name; + if (!stream->isatty ()) + { + fprintf_unfiltered (stream, "%s...", meter.name.c_str ()); + gdb_flush (stream); + meter.printing =3D WORKING; + } + else + { + /* Don't actually emit anything until the first call notifies us + of progress. This makes it so a second progress message can + be started before the first one has been notified, without + messy output. */ + meter.printing =3D should_print ? START : NO_PRINT; + } + + m_meters.push_back (std::move (meter)); +} + +void +cli_ui_out::do_progress_notify (double howmuch) +{ + struct ui_file *stream =3D m_streams.back (); + cli_progress_info &meter (m_meters.back ()); + + if (meter.printing =3D=3D NO_PRINT) + return; + + if (meter.printing =3D=3D START) + { + fprintf_unfiltered (stream, "%s\n", meter.name.c_str ()); + gdb_flush (stream); + meter.printing =3D WORKING; + } + + if (meter.printing =3D=3D WORKING && howmuch >=3D 1.0) + return; + + if (!stream->isatty ()) + return; + + int chars_per_line =3D get_chars_per_line (); + if (chars_per_line > 0) + { + int i, max; + int width =3D chars_per_line - 3; + + max =3D width * howmuch; + fprintf_unfiltered (stream, "\r["); + for (i =3D 0; i < width; ++i) + fprintf_unfiltered (stream, i < max ? "#" : " "); + fprintf_unfiltered (stream, "]"); + gdb_flush (stream); + meter.printing =3D PROGRESS; + } +} + +void +cli_ui_out::do_progress_end () +{ + struct ui_file *stream =3D m_streams.back (); + cli_progress_info &meter =3D m_meters.back (); + + if (!stream->isatty ()) + { + fprintf_unfiltered (stream, "done.\n"); + gdb_flush (stream); + } + else if (meter.printing =3D=3D PROGRESS) + { + int i; + int width =3D get_chars_per_line () - 3; + + fprintf_unfiltered (stream, "\r"); + for (i =3D 0; i < width + 2; ++i) + fprintf_unfiltered (stream, " "); + fprintf_unfiltered (stream, "\r"); + gdb_flush (stream); + } + + m_meters.pop_back (); +} + /* local functions */ =20 void diff --git a/gdb/cli-out.h b/gdb/cli-out.h index 84e957ca89..5f55554fdb 100644 --- a/gdb/cli-out.h +++ b/gdb/cli-out.h @@ -71,6 +71,10 @@ class cli_ui_out : public ui_out virtual void do_flush () override; virtual void do_redirect (struct ui_file *outstream) override; =20 + virtual void do_progress_start (const std::string &, bool) override; + virtual void do_progress_notify (double) override; + virtual void do_progress_end () override; + bool suppress_output () { return m_suppress_output; } =20 @@ -80,6 +84,33 @@ class cli_ui_out : public ui_out =20 std::vector m_streams; bool m_suppress_output; + + /* Represents the printing state of a progress meter. */ + enum meter_state + { + /* Printing will start with the next output. */ + START, + /* Printing has already started. */ + WORKING, + /* Progress printing has already started. */ + PROGRESS, + /* Printing should not be done. */ + NO_PRINT + }; + + /* The state of a recent progress meter. */ + struct cli_progress_info + { + /* The current state. */ + enum meter_state printing; + /* The name to print. */ + std::string name; + /* The last notification value. */ + double last_value; + }; + + /* Stack of progress meters. */ + std::vector m_meters; }; =20 extern cli_ui_out *cli_out_new (struct ui_file *stream); diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h index de4b3e01a4..da8f4e6457 100644 --- a/gdb/mi/mi-out.h +++ b/gdb/mi/mi-out.h @@ -83,6 +83,18 @@ class mi_ui_out : public ui_out virtual bool do_is_mi_like_p () const override { return true; } =20 + virtual void do_progress_start (const std::string &, bool) override + { + } + + virtual void do_progress_notify (double) override + { + } + + virtual void do_progress_end () override + { + } + private: =20 void field_separator (); diff --git a/gdb/ui-out.h b/gdb/ui-out.h index 9fc60614d6..dfd9679e4c 100644 --- a/gdb/ui-out.h +++ b/gdb/ui-out.h @@ -275,6 +275,39 @@ class ui_out escapes. */ virtual bool can_emit_style_escape () const =3D 0; =20 + /* An object that starts and finishes a progress meter. */ + class progress_meter + { + public: + /* SHOULD_PRINT indicates whether something should be printed for a = tty. */ + progress_meter (struct ui_out *uiout, const std::string &name, + bool should_print) + : m_uiout (uiout) + { + m_uiout->do_progress_start (name, should_print); + } + + ~progress_meter () + { + m_uiout->do_progress_notify (1.0); + m_uiout->do_progress_end (); + } + + progress_meter (const progress_meter &) =3D delete; + progress_meter &operator=3D (const progress_meter &) =3D delete; + + private: + + struct ui_out *m_uiout; + }; + + /* Emit some progress corresponding to the most recently created + progress meter. HOWMUCH may range from 0.0 to 1.0. */ + void progress (double howmuch) + { + do_progress_notify (howmuch); + } + protected: =20 virtual void do_table_begin (int nbrofcols, int nr_rows, const char *t= blid) @@ -309,6 +342,10 @@ class ui_out virtual void do_flush () =3D 0; virtual void do_redirect (struct ui_file *outstream) =3D 0; =20 + virtual void do_progress_start (const std::string &, bool) =3D 0; + virtual void do_progress_notify (double) =3D 0; + virtual void do_progress_end () =3D 0; + /* Set as not MI-like by default. It is overridden in subclasses if necessary. */ =20 diff --git a/gdb/utils.c b/gdb/utils.c index 3226656e2c..abcf6e256b 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1579,6 +1579,14 @@ gdb_flush (struct ui_file *stream) stream->flush (); } =20 +/* See utils.h. */ + +int +get_chars_per_line () +{ + return chars_per_line; +} + /* Indicate that if the next sequence of characters overflows the line, a newline should be inserted here rather than when it hits the end. If INDENT is non-null, it is a string to be printed to indent the @@ -1769,6 +1777,12 @@ fputs_maybe_filtered (const char *linebuffer, stru= ct ui_file *stream, don't increment chars_printed here. */ lineptr +=3D skip_bytes; } + else if (*lineptr =3D=3D '\r') + { + wrap_buffer.push_back (*lineptr); + chars_printed =3D 0; + lineptr++; + } else { wrap_buffer.push_back (*lineptr); diff --git a/gdb/utils.h b/gdb/utils.h index a8c65ed817..e87ce11323 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -364,6 +364,10 @@ extern void wrap_here (const char *); =20 extern void reinitialize_more_filter (void); =20 +/* Return the number of characters in a line. */ + +extern int get_chars_per_line (); + extern bool pagination_enabled; =20 extern struct ui_file **current_ui_gdb_stdout_ptr (void); --------------5D0BC89E2EF9D9C878EDD374--