[gdb/tui] Factor out border-mode help text

  I noticed that the help texts for tui border-mode and tui active-border-mode
are similar.

Factor out the common part into macro HELP_ATTRIBUTE_MODE.

Tested on x86_64-linux.
---
 gdb/tui/tui-win.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)


base-commit: f1cc4f02cb558d513cb4575211dbbb690391618f
  

On 5/21/23 21:59, Tom de Vries via Gdb-patches wrote:
> I noticed that the help texts for tui border-mode and tui active-border-mode
> are similar.
> 
> Factor out the common part into macro HELP_ATTRIBUTE_MODE.
> 

This is a v2, which uses c++ std::string instead of a macro.

OTOH, it changes the translation boundaries, and I'm not sure if the new 
parts still classify as "entire sentence".

Then again, I was not able to find any files containing translations for 
gdb, so perhaps it doesn't matter.

Thanks,
- Tom
  

On 22/05/2023 11:09, Tom de Vries via Gdb-patches wrote:
> On 5/21/23 21:59, Tom de Vries via Gdb-patches wrote:
>> I noticed that the help texts for tui border-mode and tui 
>> active-border-mode
>> are similar.
>>
>> Factor out the common part into macro HELP_ATTRIBUTE_MODE.
>>
>
> This is a v2, which uses c++ std::string instead of a macro.
>
> OTOH, it changes the translation boundaries, and I'm not sure if the 
> new parts still classify as "entire sentence".
>
> Then again, I was not able to find any files containing translations 
> for gdb, so perhaps it doesn't matter.
>
 > +  const std::string help_tui_border_mode
 > +    = (std::string ("\
 > +This variable controls the attributes to use for the window borders:\n")
 > +       + help_attribute_mode);

I think there is no need to explicitly call the std::string constructor 
here (at least compiling with gcc you don't). You could have it as:

  +  const std::string help_tui_border_mode
  +    = ("This variable controls the attributes to use for the window 
borders:\n"
  +       + help_attribute_mode);

Which makes it a bit clearer in my opinion.

Other than that (or if my suggestion isn't really feasible on all 
compilers/systems), looks ok to me:

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
  

On 5/22/23 11:09, Tom de Vries via Gdb-patches wrote:
> On 5/21/23 21:59, Tom de Vries via Gdb-patches wrote:
>> I noticed that the help texts for tui border-mode and tui 
>> active-border-mode
>> are similar.
>>
>> Factor out the common part into macro HELP_ATTRIBUTE_MODE.
>>
> 
> This is a v2, which uses c++ std::string instead of a macro.
> 
> OTOH, it changes the translation boundaries, and I'm not sure if the new 
> parts still classify as "entire sentence".
> 
> Then again, I was not able to find any files containing translations for 
> gdb, so perhaps it doesn't matter.

As there are no comments on these issues sofar, I've committed v1.

Thanks,
- Tom
  

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> OTOH, it changes the translation boundaries, and I'm not sure if the
Tom> new parts still classify as "entire sentence".

Tom> Then again, I was not able to find any files containing translations
Tom> for gdb, so perhaps it doesn't matter.

Yeah, gdb has a bunch of calls to gettext but nobody has ever gotten
translations done or installed them in the tree.  I think some of the
build machinery is missing too... hard to recall, it's been ages since I
looked at this.

Tom> +  const std::string help_attribute_mode (_("\
Tom>     normal          normal display\n\
...

Tom> +  const std::string help_tui_border_mode
Tom> +    = (std::string ("\
Tom> +This variable controls the attributes to use for the window borders:\n")
Tom> +       + help_attribute_mode);
...

The text here isn't passed through gettext.
Just wrapping the constant string in _() is enough.

Tom> +  add_setshow_enum_cmd ("border-mode", no_class, tui_border_mode_enums,
Tom> +			&tui_border_mode, _("\
Tom> +Set the attribute mode to use for the TUI window borders."), _("\
Tom> +Show the attribute mode to use for the TUI window borders."),
Tom> +			help_tui_border_mode.c_str (),

Does add_setshow_enum_cmd copy the string that's passed in?  I wouldn't
have thought so.  If not, then this can lead to a use-after-free.
Instead help_tui_border_mode would have to be 'static'.

Tom> +  const std::string help_tui_active_border_mode
Tom> +    = (std::string ("\
Tom> +This variable controls the attributes to use for the active window borders:\n")
Tom> +       + help_attribute_mode);

Here too.  And similarly with 'static' I think.

Tom
  

On 6/9/23 15:39, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> OTOH, it changes the translation boundaries, and I'm not sure if the
> Tom> new parts still classify as "entire sentence".
> 
> Tom> Then again, I was not able to find any files containing translations
> Tom> for gdb, so perhaps it doesn't matter.
> 
> Yeah, gdb has a bunch of calls to gettext but nobody has ever gotten
> translations done or installed them in the tree.  I think some of the
> build machinery is missing too... hard to recall, it's been ages since I
> looked at this.
> 
> Tom> +  const std::string help_attribute_mode (_("\
> Tom>     normal          normal display\n\
> ...
> 
> Tom> +  const std::string help_tui_border_mode
> Tom> +    = (std::string ("\
> Tom> +This variable controls the attributes to use for the window borders:\n")
> Tom> +       + help_attribute_mode);
> ...
> 
> The text here isn't passed through gettext.
> Just wrapping the constant string in _() is enough.
>

Fixed.

> Tom> +  add_setshow_enum_cmd ("border-mode", no_class, tui_border_mode_enums,
> Tom> +			&tui_border_mode, _("\
> Tom> +Set the attribute mode to use for the TUI window borders."), _("\
> Tom> +Show the attribute mode to use for the TUI window borders."),
> Tom> +			help_tui_border_mode.c_str (),
> 
> Does add_setshow_enum_cmd copy the string that's passed in?  I wouldn't
> have thought so.  If not, then this can lead to a use-after-free.

It does actually, see add_setshow_cmd_full_erased in cli-decode.c, which 
does:
...
   if (help_doc != NULL)
     {
       full_set_doc = xstrprintf ("%s\n%s", set_doc, help_doc);
       full_show_doc = xstrprintf ("%s\n%s", show_doc, help_doc);
     }
   else
     {
       full_set_doc = make_unique_xstrdup (set_doc);
       full_show_doc = make_unique_xstrdup (show_doc);
     }
...


> Instead help_tui_border_mode would have to be 'static'.
> 
> Tom> +  const std::string help_tui_active_border_mode
> Tom> +    = (std::string ("\
> Tom> +This variable controls the attributes to use for the active window borders:\n")
> Tom> +       + help_attribute_mode);
> 
> Here too.  And similarly with 'static' I think.

This version, rebased on trunk and also addressing the comment from 
Bruno about not needing explicit std::string constructors is what I'm 
planning to commit.

Thanks,
- Tom