From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42248 invoked by alias); 4 Oct 2016 19:24:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 42159 invoked by uid 89); 4 Oct 2016 19:24:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=disappear, sk:current, 6257, 1267 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Oct 2016 19:24:38 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BBD0085541; Tue, 4 Oct 2016 19:24:37 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u94JOUZX017403; Tue, 4 Oct 2016 15:24:33 -0400 Subject: Re: [RFA 19/22] Convert tid_range_parser to class To: Tom Tromey References: <1474949330-4307-1-git-send-email-tom@tromey.com> <1474949330-4307-20-git-send-email-tom@tromey.com> <55f2924c-f8f9-6c06-ffbb-69079b6ffa62@redhat.com> <87shsha3bf.fsf@tromey.com> <926126cb-b3c5-340b-ac1c-5bc14ca41bf9@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <42bd9696-cea5-5f39-ce03-1b223b5ed6fc@redhat.com> Date: Tue, 04 Oct 2016 19:24:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <926126cb-b3c5-340b-ac1c-5bc14ca41bf9@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00067.txt.bz2 On 10/01/2016 12:23 AM, Pedro Alves wrote: > On 09/30/2016 03:07 PM, Tom Tromey wrote: >>>>>>> "Pedro" == Pedro Alves writes: >> >> Pedro> Whoops, we ended up duplicating work here. >> >> Oops :) But no big deal. >> >> Pedro> Additional differences compared to yours are: >> Pedro> - "bool" instead of "int". >> Pedro> - "m_" prefixes. >> >> Aha, the m_ prefix. Cancel that earlier question... >> >> Pedro> Let me know what you think. It's super fine with me to rebase >> Pedro> mine on top of yours. Did you have a follow up patch for >> Pedro> get_number_or_range too, perhaps? >> >> I don't. How about I just drop my patch? That seems simplest to me. > > OK. I'll look at your patch in more detail and see what could be > merged in. I'll add your name to the ChangeLog. > >> >> Pedro> void >> Pedro> -init_number_or_range (struct get_number_or_range_state *state, >> Pedro> - const char *string) >> Pedro> +number_or_range_parser::init (const char *string) >> Pedro> { >> Pedro> - memset (state, 0, sizeof (*state)); >> Pedro> - state->string = string; >> Pedro> + memset (this, 0, sizeof (*this)); >> >> I think it's better to do explicit initialization. > > I'll do this. > >> This will bite if this class ever is changed to have a vtable (unlikely >> but it's better, IMO, to set a good example). > > Yeah, we'd notice bad things immediately, but I agree with setting > a good example. I did the change below locally. I was looking at string() vs get_string(), and not being super happy with either. string() is maybe too generic, doesn't really convey what the string is about, and the get_ in get_string() makes me go "the other get_ members are about parsing/producing a number, while this is peeking at parser state". So in the style of xkcd 927, I thought of an alternative name -- cur_tok(), which seems to be at least somewhat of a common field/method name in parser lingo, according to google. I've also renamed get_next to get_number: - int gotnum = parser.get_next (); + int gotnum = parser.get_number (); which seems more consistent. Got rid of the memset too. I'm now looking at finished() vs is_qualified() and pondering whether to do something about that little inconsistency. :-) Anyway, I have to disappear now. Will continue later. >From 4ae2903eee5b5c04f56658a02c486ba4c8edcd5f Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 4 Oct 2016 20:14:56 +0100 Subject: [PATCH] address review --- gdb/breakpoint.c | 6 +++--- gdb/cli/cli-utils.c | 28 ++++++++++++++++------------ gdb/cli/cli-utils.h | 18 +++++++++--------- gdb/inferior.c | 6 +++--- gdb/linespec.c | 2 +- gdb/memattr.c | 6 +++--- gdb/printcmd.c | 4 ++-- gdb/reverse.c | 4 ++-- gdb/thread.c | 7 +++---- gdb/tid-parse.c | 32 ++++++++++++++++---------------- gdb/tid-parse.h | 12 ++++++------ 11 files changed, 64 insertions(+), 61 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 6b47541..32d6a95 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -14783,10 +14783,10 @@ map_breakpoint_numbers (char *args, void (*function) (struct breakpoint *, while (!parser.finished ()) { - const char *p = parser.string (); + const char *p = parser.cur_tok (); bool match = false; - num = parser.get_next (); + num = parser.get_number (); if (num == 0) { warning (_("bad breakpoint number at or near '%s'"), p); @@ -15639,7 +15639,7 @@ get_tracepoint_by_number (char **arg, if (parser != NULL) { gdb_assert (!parser->finished ()); - tpnum = parser->get_next (); + tpnum = parser->get_number (); } else if (arg == NULL || *arg == NULL || ! **arg) tpnum = tracepoint_count; diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c index 379f341..0fb68f2 100644 --- a/gdb/cli/cli-utils.c +++ b/gdb/cli/cli-utils.c @@ -131,14 +131,18 @@ number_or_range_parser::number_or_range_parser (const char *string) void number_or_range_parser::init (const char *string) { - memset (this, 0, sizeof (*this)); - m_string = string; + m_finished = false; + m_cur_tok = string; + m_last_retval = 0; + m_end_value = 0; + m_end_ptr = NULL; + m_in_range = false; } /* See documentation in cli-utils.h. */ int -number_or_range_parser::get_next () +number_or_range_parser::get_number () { if (m_in_range) { @@ -150,16 +154,16 @@ number_or_range_parser::get_next () if (++m_last_retval == m_end_value) { /* End of range reached; advance token pointer. */ - m_string = m_end_ptr; + m_cur_tok = m_end_ptr; m_in_range = false; } } - else if (*m_string != '-') + else if (*m_cur_tok != '-') { - /* Default case: state->string is pointing either to a solo + /* Default case: state->m_cur_tok is pointing either to a solo number, or to the first number of a range. */ - m_last_retval = get_number_trailer (&m_string, '-'); - if (*m_string == '-') + m_last_retval = get_number_trailer (&m_cur_tok, '-'); + if (*m_cur_tok == '-') { const char **temp; @@ -168,7 +172,7 @@ number_or_range_parser::get_next () and also remember the end of the final token. */ temp = &m_end_ptr; - m_end_ptr = skip_spaces_const (m_string + 1); + m_end_ptr = skip_spaces_const (m_cur_tok + 1); m_end_value = get_number_const (temp); if (m_end_value < m_last_retval) { @@ -179,7 +183,7 @@ number_or_range_parser::get_next () /* Degenerate range (number1 == number2). Advance the token pointer so that the range will be treated as a single number. */ - m_string = m_end_ptr; + m_cur_tok = m_end_ptr; } else m_in_range = true; @@ -187,7 +191,7 @@ number_or_range_parser::get_next () } else error (_("negative value")); - m_finished = *m_string == '\0'; + m_finished = *m_cur_tok == '\0'; return m_last_retval; } @@ -222,7 +226,7 @@ number_is_in_list (const char *list, int number) number_or_range_parser parser (list); while (!parser.finished ()) { - int gotnum = parser.get_next (); + int gotnum = parser.get_number (); if (gotnum == 0) error (_("Args must be numbers or '$' variables.")); diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h index 41c3a58..ab3f122 100644 --- a/gdb/cli/cli-utils.h +++ b/gdb/cli/cli-utils.h @@ -62,11 +62,11 @@ public: each call it will return the next value in the range. At the beginning of parsing a range, the char pointer - STATE->string will be advanced past and left pointing - at the '-' token. Subsequent calls will not advance the pointer - until the range is completed. The call that completes the range - will advance the pointer past . */ - int get_next (); + STATE->m_cur_tok will be advanced past and left + pointing at the '-' token. Subsequent calls will not advance the + pointer until the range is completed. The call that completes + the range will advance the pointer past . */ + int get_number (); /* Setup internal state such that get_next() returns numbers in the START_VALUE to END_VALUE range. END_PTR is where the string is @@ -80,8 +80,8 @@ public: /* Return the string being parsed. When parsing has finished, this points past the last parsed token. */ - const char *string () - { return m_string; } + const char *cur_tok () + { return m_cur_tok; } /* True when parsing a range. */ bool in_range () @@ -95,7 +95,7 @@ public: void skip_range () { gdb_assert (m_in_range); - m_string = m_end_ptr; + m_cur_tok = m_end_ptr; } private: @@ -104,7 +104,7 @@ private: /* The string being parsed. When parsing has finished, this points past the last parsed token. */ - const char *m_string; + const char *m_cur_tok; /* Last value returned. */ int m_last_retval; diff --git a/gdb/inferior.c b/gdb/inferior.c index 836adc9..1602483 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -659,7 +659,7 @@ detach_inferior_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); if (!valid_gdb_inferior_id (num)) { @@ -698,7 +698,7 @@ kill_inferior_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); if (!valid_gdb_inferior_id (num)) { @@ -790,7 +790,7 @@ remove_inferior_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); struct inferior *inf = find_inferior_id (num); if (inf == NULL) diff --git a/gdb/linespec.c b/gdb/linespec.c index 7b1a77e..5182b45 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -1411,7 +1411,7 @@ decode_line_2 (struct linespec_state *self, number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); if (num == 0) error (_("canceled")); diff --git a/gdb/memattr.c b/gdb/memattr.c index a33993a..1c5c48f 100644 --- a/gdb/memattr.c +++ b/gdb/memattr.c @@ -581,7 +581,7 @@ mem_enable_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - num = parser.get_next (); + num = parser.get_number (); mem_enable (num); } } @@ -625,7 +625,7 @@ mem_disable_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); mem_disable (num); } } @@ -676,7 +676,7 @@ mem_delete_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); mem_delete (num); } diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 15f2395..c7f477b 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -1885,9 +1885,9 @@ map_display_numbers (char *args, while (!parser.finished ()) { - const char *p = parser.string (); + const char *p = parser.cur_tok (); - num = parser.get_next (); + num = parser.get_number (); if (num == 0) warning (_("bad display number at or near '%s'"), p); else diff --git a/gdb/reverse.c b/gdb/reverse.c index 461d1a4..2c1abdc 100644 --- a/gdb/reverse.c +++ b/gdb/reverse.c @@ -233,7 +233,7 @@ delete_bookmark_command (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int num = parser.get_next (); + int num = parser.get_number (); if (!delete_one_bookmark (num)) /* Not found. */ warning (_("No bookmark #%d."), num); @@ -329,7 +329,7 @@ bookmarks_info (char *args, int from_tty) number_or_range_parser parser (args); while (!parser.finished ()) { - int bnum = parser.get_next (); + int bnum = parser.get_number (); bookmark_1 (bnum); } } diff --git a/gdb/thread.c b/gdb/thread.c index 819119d..f376211 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1837,7 +1837,7 @@ thread_apply_command (char *tidlist, int from_tty) if (!parser.get_tid_range (&inf_num, &thr_start, &thr_end)) { - cmd = (char *) parser.string (); + cmd = (char *) parser.cur_tok (); break; } } @@ -1856,7 +1856,7 @@ thread_apply_command (char *tidlist, int from_tty) make_cleanup_restore_current_thread (); parser.init (tidlist, current_inferior ()->num); - while (!parser.finished () && parser.string () < cmd) + while (!parser.finished () && parser.cur_tok () < cmd) { struct thread_info *tp = NULL; struct inferior *inf; @@ -1888,8 +1888,7 @@ thread_apply_command (char *tidlist, int from_tty) if (tp == NULL) { - if (show_inferior_qualified_tids () - || parser.qualified ()) + if (show_inferior_qualified_tids () || parser.is_qualified ()) warning (_("Unknown thread %d.%d"), inf_num, thr_num); else warning (_("Unknown thread %d"), thr_num); diff --git a/gdb/tid-parse.c b/gdb/tid-parse.c index 9e49eba..28c5286 100644 --- a/gdb/tid-parse.c +++ b/gdb/tid-parse.c @@ -125,7 +125,7 @@ void tid_range_parser::init (const char *tidlist, int default_inferior) { m_state = STATE_INFERIOR; - m_string = tidlist; + m_cur_tok = tidlist; m_inf_num = 0; m_qualified = 0; m_default_inferior = default_inferior; @@ -139,7 +139,7 @@ tid_range_parser::finished () switch (m_state) { case STATE_INFERIOR: - return *m_string == '\0'; + return *m_cur_tok == '\0'; case STATE_THREAD_RANGE: case STATE_STAR_RANGE: return m_range_parser.finished (); @@ -151,15 +151,15 @@ tid_range_parser::finished () /* See tid-parse.h. */ const char * -tid_range_parser::string () +tid_range_parser::cur_tok () { switch (m_state) { case STATE_INFERIOR: - return m_string; + return m_cur_tok; case STATE_THREAD_RANGE: case STATE_STAR_RANGE: - return m_range_parser.string (); + return m_range_parser.cur_tok (); } gdb_assert_not_reached (_("unhandled state")); @@ -172,13 +172,13 @@ tid_range_parser::skip_range () || m_state == STATE_STAR_RANGE); m_range_parser.skip_range (); - init (m_range_parser.string (), m_default_inferior); + init (m_range_parser.cur_tok (), m_default_inferior); } /* See tid-parse.h. */ bool -tid_range_parser::qualified () +tid_range_parser::is_qualified () { return m_qualified; } @@ -196,9 +196,9 @@ tid_range_parser::get_tid_or_range (int *inf_num, const char *p; const char *space; - space = skip_to_space (m_string); + space = skip_to_space (m_cur_tok); - p = m_string; + p = m_cur_tok; while (p < space && *p != '.') p++; if (p < space) @@ -206,8 +206,8 @@ tid_range_parser::get_tid_or_range (int *inf_num, const char *dot = p; /* Parse number to the left of the dot. */ - p = m_string; - m_inf_num = get_positive_number_trailer (&p, '.', m_string); + p = m_cur_tok; + m_inf_num = get_positive_number_trailer (&p, '.', m_cur_tok); if (m_inf_num == 0) return 0; @@ -221,7 +221,7 @@ tid_range_parser::get_tid_or_range (int *inf_num, { m_inf_num = m_default_inferior; m_qualified = false; - p = m_string; + p = m_cur_tok; } m_range_parser.init (p); @@ -237,9 +237,9 @@ tid_range_parser::get_tid_or_range (int *inf_num, } *inf_num = m_inf_num; - *thr_start = m_range_parser.get_next (); + *thr_start = m_range_parser.get_number (); if (*thr_start < 0) - error (_("negative value: %s"), m_string); + error (_("negative value: %s"), m_cur_tok); if (*thr_start == 0) { m_state = STATE_INFERIOR; @@ -252,7 +252,7 @@ tid_range_parser::get_tid_or_range (int *inf_num, if (!m_range_parser.in_range ()) { m_state = STATE_INFERIOR; - m_string = m_range_parser.string (); + m_cur_tok = m_range_parser.cur_tok (); if (thr_end != NULL) *thr_end = *thr_start; @@ -316,7 +316,7 @@ tid_is_in_list (const char *list, int default_inferior, int tmp_inf, tmp_thr_start, tmp_thr_end; if (!parser.get_tid_range (&tmp_inf, &tmp_thr_start, &tmp_thr_end)) - invalid_thread_id_error (parser.string ()); + invalid_thread_id_error (parser.cur_tok ()); if (tmp_inf == inf_num && tmp_thr_start <= thr_num && thr_num <= tmp_thr_end) return 1; diff --git a/gdb/tid-parse.h b/gdb/tid-parse.h index 764facc..4ff74b7 100644 --- a/gdb/tid-parse.h +++ b/gdb/tid-parse.h @@ -74,7 +74,7 @@ public: exists). At the beginning of parsing a thread range, the char pointer - PARSER->string will be advanced past and left + PARSER->m_cur_tok will be advanced past and left pointing at the '-' token. Subsequent calls will not advance the pointer until the range is completed. The call that completes the range will advance the pointer past . @@ -116,9 +116,9 @@ public: /* Returns true if parsing has completed. */ bool finished (); - /* Return the string being parsed. When parsing has finished, this - points past the last parsed token. */ - const char *string (); + /* Return the current token being parsed. When parsing has + finished, this points past the last parsed token. */ + const char *cur_tok (); /* When parsing a range, advance past the final token in the range. */ @@ -126,7 +126,7 @@ public: /* True if the TID last parsed was explicitly inferior-qualified. IOW, whether the spec specified an inferior number explicitly. */ - bool qualified (); + bool is_qualified (); private: bool get_tid_or_range (int *inf_num, int *thr_start, int *thr_end); @@ -147,7 +147,7 @@ private: /* The string being parsed. When parsing has finished, this points past the last parsed token. */ - const char *m_string; + const char *m_cur_tok; /* The range parser state when we're parsing the thread number sub-component. */ -- 2.5.5