Re: [PATCH 23/40] Make language_def O(1)

[Keith Seitz <keiths@redhat.com>]

On 06/02/2017 05:22 AM, Pedro Alves wrote:
> IMO, the add_language mechanism is pointless, because "enum language"
> implies the core of GDB needs to know about all languages anyway.

I don't know how others feel about it, but since I am giving feedback on this patch, I will say that I agree with you.

Now, if there was a mechanism by which we could dynamically add languages, add_language would make much more sense.

> Note that "local_language_defn" is gone along the way.  AFAICT, it's
> just a copy of "auto", so the new code simply maps one to the other.
> One fewer place to update when we need to change the language
> vector...

I've searched the manual, and the "local" language is mentioned only in two places, once in the description of "set language" (where it explains that "local" and "auto" are the same) and once when we describe -data-evaluate-expression's --language flag.

Honestly, is there any reason to keep it at all given it is a synonym of auto? [I realize that it doesn't cost much to maintain with this patch.] I'm not asking for any changes in this regard, just throwing the possibility out there. TBH, I didn't even know "local" existed.

Just two trivial nits.

> diff --git a/gdb/language.c b/gdb/language.c
> index d30f4f0..df4f3cd 100644
> --- a/gdb/language.c
> +++ b/gdb/language.c
> @@ -530,55 +522,51 @@ show_check (char *ignore, int from_tty)
>    cmd_show_list (showchecklist, from_tty, "");
>  }
>  \f
> -/* Add a language to the set of known languages.  */
>  
> -void
> -add_language (const struct language_defn *lang)
> +/* Compare C strings for std::sort.  */
> +
> +static bool
> +compare_cstrings (const char *str1, const char *str2)
>  {
> -  /* For the "set language" command.  */
> -  static const char **language_names = NULL;
> -  /* For the "help set language" command.  */
> +  return strcmp (str1, str2) < 0;
> +}

Doesn't completer.c define the same function? Perhaps move to utils.c? I would guess this is not the last time we will want to do this "sort" of thing. :-)

>  
> -  if (lang->la_magic != LANG_MAGIC)
> -    {
> -      fprintf_unfiltered (gdb_stderr,
> -			  "Magic number of %s language struct wrong\n",
> -			  lang->la_name);
> -      internal_error (__FILE__, __LINE__,
> -		      _("failed internal consistency check"));
> -    }
> +static void
> +build_language_commands ()
> +{

missing comment<cough/>

> diff --git a/gdb/testsuite/gdb.base/default.exp b/gdb/testsuite/gdb.base/default.exp
> index 8c0b29c..c25069a 100644
> --- a/gdb/testsuite/gdb.base/default.exp
> +++ b/gdb/testsuite/gdb.base/default.exp
> @@ -511,7 +511,7 @@ gdb_test "set history size" "Argument required .integer to set it to.*" "set his
>  #test set history
>  gdb_test "set history" "\"set history\" must be followed by the name of a history subcommand.(\[^\r\n\]*\[\r\n\])+List of set history subcommands:(\[^\r\n\]*\[\r\n\])+set history expansion -- Set history expansion on command input(\[^\r\n\]*\[\r\n\])+set history filename -- Set the filename in which to record the command history(\[^\r\n\]*\[\r\n\])+set history save -- Set saving of the history record on exit(\[^\r\n\]*\[\r\n\])+set history size -- Set the size of the command history(\[^\r\n\]*\[\r\n\])+Type \"help set history\" followed by set history subcommand name for full documentation.(\[^\r\n\]*\[\r\n\])+Command name abbreviations are allowed if unambiguous." "set history"
>  #test set language
> -gdb_test "set language" "Requires an argument. Valid arguments are ada, c, c.., asm, minimal, d, fortran, go, auto, local, unknown, modula-2, objective-c, opencl, pascal, rust." "set language"
> +gdb_test "set language" "Requires an argument. Valid arguments are auto, local, unknown, ada, asm, c, c.., d, fortran, go, minimal, modula-2, objective-c, opencl, pascal, rust." "set language"

Nice catch.

Otherwise, LGTM.

Keith