[PATCH 1/2] [gdb/tui] Use const std::string for string literals in tui-stack.c

[PATCH 1/2] [gdb/tui] Use const std::string for string literals in tui-stack.c

[Tom de Vries]

I noticed in gdb/tui/tui-stack.c a source-level micro-optimization where
strlen with a string literal argument:
...
strlen ("bla")
...
is replaced with sizeof:
...
sizeof ("bla") - 1
...

The benefit of this is that the optimization is also done at O0, but the
drawback is that it makes expression harder to read.

Use const std::string to encapsulate the string literals, and use
std::string::size () instead.

I tried making the string names (PROC_PREFIX, LINE_PREFIX, PC_PREFIX and
SINGLE_KEY) lower-case, but that clashed with a pre-existing pc_prefix, so
I've them upper-case.

Tested on x86_64-linux.
---
 gdb/tui/tui-stack.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index 76b8f066abb..723d6268aad 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -40,12 +40,12 @@
 
 #include "gdb_curses.h"
 
-#define PROC_PREFIX             "In: "
-#define LINE_PREFIX             "L"
-#define PC_PREFIX               "PC: "
+static const std::string PROC_PREFIX = "In: ";
+static const std::string LINE_PREFIX = "L";
+static const std::string PC_PREFIX = "PC: ";
 
 /* Strings to display in the TUI status line.  */
-#define SINGLE_KEY              "(SingleKey)"
+static const std::string SINGLE_KEY = "(SingleKey)";
 
 /* Minimum/Maximum length of some fields displayed in the TUI status
    line.  */
@@ -107,16 +107,15 @@ tui_locator_window::make_status_line () const
   int pc_width = pc_out.size ();
 
   /* First determine the amount of proc name width we have available.
-     The +1 are for a space separator between fields.
-     The -1 are to take into account the \0 counted by sizeof.  */
+     The +1 are for a space separator between fields.  */
   proc_width = (status_size
 		- (target_width + 1)
 		- (pid_width + 1)
-		- (sizeof (PROC_PREFIX) - 1 + 1)
-		- (sizeof (LINE_PREFIX) - 1 + line_width + 1)
-		- (sizeof (PC_PREFIX) - 1 + pc_width + 1)
+		- (PROC_PREFIX.size () + 1)
+		- (LINE_PREFIX.size () + line_width + 1)
+		- (PC_PREFIX.size () + pc_width + 1)
 		- (tui_current_key_mode == TUI_SINGLE_KEY_MODE
-		   ? (sizeof (SINGLE_KEY) - 1 + 1)
+		   ? (SINGLE_KEY.size () + 1)
 		   : 0));
 
   /* If there is no room to print the function name, try by removing
@@ -131,11 +130,11 @@ tui_locator_window::make_status_line () const
 	  pid_width = 0;
 	  if (proc_width <= MIN_PROC_WIDTH)
 	    {
-	      proc_width += pc_width + sizeof (PC_PREFIX) - 1 + 1;
+	      proc_width += pc_width + PC_PREFIX.size () + 1;
 	      pc_width = 0;
 	      if (proc_width < 0)
 		{
-		  proc_width += line_width + sizeof (LINE_PREFIX) - 1 + 1;
+		  proc_width += line_width + LINE_PREFIX.size () + 1;
 		  line_width = 0;
 		  if (proc_width < 0)
 		    proc_width = 0;
@@ -156,7 +155,7 @@ tui_locator_window::make_status_line () const
   /* Show whether we are in SingleKey mode.  */
   if (tui_current_key_mode == TUI_SINGLE_KEY_MODE)
     {
-      string.puts (SINGLE_KEY);
+      string.puts (SINGLE_KEY.c_str ());
       string.puts (" ");
     }
 
@@ -165,19 +164,19 @@ tui_locator_window::make_status_line () const
     {
       const std::string &proc_name = tui_location.proc_name ();
       if (proc_name.size () > proc_width)
-	string.printf ("%s%*.*s* ", PROC_PREFIX,
+	string.printf ("%s%*.*s* ", PROC_PREFIX.c_str (),
 		       1 - proc_width, proc_width - 1, proc_name.c_str ());
       else
-	string.printf ("%s%*.*s ", PROC_PREFIX,
+	string.printf ("%s%*.*s ", PROC_PREFIX.c_str (),
 		       -proc_width, proc_width, proc_name.c_str ());
     }
 
   if (line_width > 0)
-    string.printf ("%s%*.*s ", LINE_PREFIX,
+    string.printf ("%s%*.*s ", LINE_PREFIX.c_str (),
 		   -line_width, line_width, line_buf);
   if (pc_width > 0)
     {
-      string.puts (PC_PREFIX);
+      string.puts (PC_PREFIX.c_str ());
       string.puts (pc_buf);
     }
 

base-commit: 476bf7d5e6661b06eb9f8de9258cf48fd81919af
-- 
2.35.3

[PATCH 2/2] [gdb/tui] Show focus window in status line

[Tom de Vries]

The focused window is highlighting by using active-border-kind instead of
border-kind.

But if the focused window is the cmd window (which is an unboxed window), then
no highlighting is done, and it's not obvious from looking at the screen which
window has the focus.  Instead, you have to notice the absence of highlighting
on boxed windows, and then infer that the focus is on the unboxed window.

That approach stops working if there are multiple unboxed windows.

Likewise if highlighting is switched off by setting active-border-kind to the
same value as border-kind.

Make it more explicit which window has the focus by mentioning it in the status
window, like so:
...
native process 8282 (src) In: main                      L7    PC: 0x400525
...

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.tui/single-key.exp |  4 ++--
 gdb/tui/tui-data.c                   |  2 ++
 gdb/tui/tui-stack.c                  | 16 ++++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/single-key.exp b/gdb/testsuite/gdb.tui/single-key.exp
index 07c9b00cce5..3925a8142c1 100644
--- a/gdb/testsuite/gdb.tui/single-key.exp
+++ b/gdb/testsuite/gdb.tui/single-key.exp
@@ -25,12 +25,12 @@ set status_win { 0 15 80 1 }
 set command_win { 0 16 80 8 }
 
 # Check that the status window doesn't show Singlekey.
-set re "No process In:"
+set re [string_to_regexp "No process (src) In:"]
 Term::check_region_contents  "status window: initial" {*}$status_win $re
 
 # Enter single-key mode.  Check that the status window does show Singlekey.
 send_gdb "\030s"
-set re "No process \\(SingleKey\\) In:"
+set re [string_to_regexp "No process (SingleKey) (src) In:"]
 gdb_assert { [Term::wait_for_region_contents {*}$status_win $re] } \
     "status window: single-key mode"
 
diff --git a/gdb/tui/tui-data.c b/gdb/tui/tui-data.c
index fc90df25ddd..5e05cccf603 100644
--- a/gdb/tui/tui-data.c
+++ b/gdb/tui/tui-data.c
@@ -26,6 +26,7 @@
 #include "tui/tui-win.h"
 #include "tui/tui-wingeneral.h"
 #include "tui/tui-winsource.h"
+#include "tui/tui-stack.h"
 #include "gdb_curses.h"
 #include <algorithm>
 
@@ -69,6 +70,7 @@ tui_set_win_focus_to (struct tui_win_info *win_info)
       tui_unhighlight_win (win_with_focus);
       win_with_focus = win_info;
       tui_highlight_win (win_info);
+      tui_show_locator_content ();
     }
 }
 
diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c
index 723d6268aad..8bf65ea3556 100644
--- a/gdb/tui/tui-stack.c
+++ b/gdb/tui/tui-stack.c
@@ -106,6 +106,12 @@ tui_locator_window::make_status_line () const
   const char *pc_buf = pc_out.c_str ();
   int pc_width = pc_out.size ();
 
+  /* Width of the field showing the window with current focus.  For a window
+     named "src" we show "(src)".  */
+  int focus_width = (tui_win_with_focus () != nullptr
+		     ? 1 + strlen (tui_win_with_focus ()->name ()) + 1
+		     : 0);
+
   /* First determine the amount of proc name width we have available.
      The +1 are for a space separator between fields.  */
   proc_width = (status_size
@@ -116,6 +122,9 @@ tui_locator_window::make_status_line () const
 		- (PC_PREFIX.size () + pc_width + 1)
 		- (tui_current_key_mode == TUI_SINGLE_KEY_MODE
 		   ? (SINGLE_KEY.size () + 1)
+		   : 0)
+		- (focus_width > 0
+		   ? focus_width + 1
 		   : 0));
 
   /* If there is no room to print the function name, try by removing
@@ -159,6 +168,13 @@ tui_locator_window::make_status_line () const
       string.puts (" ");
     }
 
+  if (tui_win_with_focus () != nullptr)
+    {
+      string.puts ("(");
+      string.puts (tui_win_with_focus ()->name ());
+      string.puts (") ");
+    }
+
   /* Procedure/class name.  */
   if (proc_width > 0)
     {
-- 
2.35.3

Re: [PATCH 2/2] [gdb/tui] Show focus window in status line

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Mon, Nov 27, 2023 at 4:48 AM Tom de Vries <tdevries@suse.de> wrote:

> The focused window is highlighting by using active-border-kind instead of
> border-kind.
>
> But if the focused window is the cmd window (which is an unboxed window),
> then
> no highlighting is done, and it's not obvious from looking at the screen
> which
> window has the focus.  Instead, you have to notice the absence of
> highlighting
> on boxed windows, and then infer that the focus is on the unboxed window.
>
> That approach stops working if there are multiple unboxed windows.
>
> Likewise if highlighting is switched off by setting active-border-kind to
> the
> same value as border-kind.
>
> Make it more explicit which window has the focus by mentioning it in the
> status
> window, like so:
> ...
> native process 8282 (src) In: main                      L7    PC: 0x400525
> ...
>
> Tested on x86_64-linux.
> ---
> I can confirm it works for ppc64le Fedora Rawhide, the (src) is neatly
> there. Causes no regressions.
>

Re: [PATCH 2/2] [gdb/tui] Show focus window in status line

[Tom de Vries]

On 11/28/23 16:22, Alexandra Petlanova Hajkova wrote:
> 
> 
> On Mon, Nov 27, 2023 at 4:48 AM Tom de Vries <tdevries@suse.de 
> <mailto:tdevries@suse.de>> wrote:
> 
>     The focused window is highlighting by using active-border-kind

Hi,

I see now this should be "highlighted".

>     instead of
>     border-kind.
> 
>     But if the focused window is the cmd window (which is an unboxed
>     window), then
>     no highlighting is done, and it's not obvious from looking at the
>     screen which
>     window has the focus.  Instead, you have to notice the absence of
>     highlighting
>     on boxed windows, and then infer that the focus is on the unboxed
>     window.
> 
>     That approach stops working if there are multiple unboxed windows.
> 
>     Likewise if highlighting is switched off by setting
>     active-border-kind to the
>     same value as border-kind.
> 
>     Make it more explicit which window has the focus by mentioning it in
>     the status
>     window, like so:
>     ...
>     native process 8282 (src) In: main                      L7    PC:
>     0x400525
>     ...
> 
>     Tested on x86_64-linux.
>     ---
>     I can confirm it works for ppc64le Fedora Rawhide, the (src) is
>     neatly there. Causes no regressions.
> 

Thanks for testing this.

Since this is user-visible, I'll leave this for Tom to comment.

Thanks,
- Tom

Re: [PATCH 1/2] [gdb/tui] Use const std::string for string literals in tui-stack.c

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Mon, Nov 27, 2023 at 4:39 AM Tom de Vries <tdevries@suse.de> wrote:

> I noticed in gdb/tui/tui-stack.c a source-level micro-optimization where
> strlen with a string literal argument:
> ...
> strlen ("bla")
> ...
> is replaced with sizeof:
> ...
> sizeof ("bla") - 1
> ...
>
> The benefit of this is that the optimization is also done at O0, but the
> drawback is that it makes expression harder to read.
>
> Use const std::string to encapsulate the string literals, and use
> std::string::size () instead.
>
> I tried making the string names (PROC_PREFIX, LINE_PREFIX, PC_PREFIX and
> SINGLE_KEY) lower-case, but that clashed with a pre-existing pc_prefix, so
> I've them upper-case.
>
> Tested on x86_64-linux.
>
>
The last paragraph of the commit message sounds a bit confusing, I mean
"I've them upper-case".
Maybe you could rephrase it?
I can confirm  the change adds no regressions on ppc64le Fedora Rawhide.

Re: [PATCH 1/2] [gdb/tui] Use const std::string for string literals in tui-stack.c

[Tom de Vries]

On 11/28/23 10:19, Alexandra Petlanova Hajkova wrote:
> 
> 
> On Mon, Nov 27, 2023 at 4:39 AM Tom de Vries <tdevries@suse.de 
> <mailto:tdevries@suse.de>> wrote:
> 
>     I noticed in gdb/tui/tui-stack.c a source-level micro-optimization where
>     strlen with a string literal argument:
>     ...
>     strlen ("bla")
>     ...
>     is replaced with sizeof:
>     ...
>     sizeof ("bla") - 1
>     ...
> 
>     The benefit of this is that the optimization is also done at O0, but the
>     drawback is that it makes expression harder to read.
> 
>     Use const std::string to encapsulate the string literals, and use
>     std::string::size () instead.
> 
>     I tried making the string names (PROC_PREFIX, LINE_PREFIX, PC_PREFIX and
>     SINGLE_KEY) lower-case, but that clashed with a pre-existing
>     pc_prefix, so
>     I've them upper-case.
> 
>     Tested on x86_64-linux.
> 
> 
> The last paragraph of the commit message sounds a bit confusing, I mean 
> "I've them upper-case".
 > Maybe you could rephrase it?

Hi,

thanks for the review.

I've added the missing "left" in the sentence, thanks for spotting that.

> I can confirm  the change adds no regressions on ppc64le Fedora Rawhide.

I've added a tested-by tag, and committed.

Thanks,
- Tom