This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch 1/2] New 'explore' command C implementation (with docs)


> Date: Wed, 7 Mar 2012 16:58:23 +0530
> From: Siva Chandra <sivachandra@google.com>
> 
> This is the implementation of the 'explore' command in C.

Thanks.

>         * doc/gdb.texinfo: Add description about the 'explore'
>         command in the chapter 'Examining Data'

This should be formatted like this:

	* gdb.texinfo (Examining Data): Document the 'explore' command.

and it should go into the doc/ChangeLog file.

>  @cindex printing data
>  @cindex examining data
> +@cindex exploring data

This index entry is not useful.  "Exploring" is too general, and on
this level is not sufficiently different from "examining".  Such an
index entry won't tell the reader what is this about.

I suggest something like

 @cindex exploring hierarchical data structures

Also, please put this index entry at the beginning of the text that
introduces this command, so that Info readers land on the correct spot
when the reader uses index-search commands.

>  @kindex print
>  @kindex inspect
> +@kindex explore

Same here: please move this @kindex entry to the beginning of the
description of 'explore'.

> +Another way of examining values of expressions and type information is
> +through the @code{explore} command.  It offers an interactive way to
> +start at the highest level (the most abstract level) of the data
> +type of an expression (or, the data type itself) and explore all the way
> +down to leaf scalar values embedded in the higher level data types.

This is too abstract, IMO.  An example of such usage will go a long
way towards making it much more clear.

> +@table @code
> +@item explore @var{arg}
> +@var{arg} is either an expression (in the source language), or a type
> +visible in the current context of the program being debugged.
> +
> +@item explore value @var{expr}
> +@cindex explore value
> +This variation lets one explicitly specify that value exploration is
> +being invoked on the expression @var{expr}.  @var{expr} should be an
> +expression valid in the current context of the program being debugged.
> +
> +@item explore type @var{arg}
> +@cindex explore type
> +This variation lets one explicitly specify that type exploration is
> +being invoked on @var{arg} which can be either be an expression or a
> +type.  If it were an expression, then the type of value of the
> +expression is explored.

Again, an example or two will clarify the differences between these
variations.  As written, the description left me wondering about the
difference.

> +  resp1 = gdb_readline (_("Do you want to explore it as an array or a single "
> +                          "value pointer [a/s] : "));
> +  make_cleanup (xfree, resp1);
> +  if (strcmp (resp1, "a") == 0)
>    ...
> +  else if (strcmp (resp1, "s") == 0)

What will happen to these hardcoded values of responses if the prompt
is translated into some language other than English?

I suggest to rephrase the question so that you could use the yes/no
facilities instead, which are easily translatable.

> +  printf_filtered (_("%s is of a pointer type pointing to a value of type "
> +                     "'%s'.\n"), path, type_name);

I would rephrase:

 %s is a pointer to a value of type '%s'

Isn't this more concise and yet accurate?

> +  printf_filtered (_("'%s' is an array of elements of type '%s'.\n"), exp_str,
> +                   elem_type_name);

Likewise here:

  '%s' is an array of '%s'

I suggest to look at the well-known 'cdecl' command for examples of
well-thought (IMO) and concise way of describing complex types.  Foor
example:

  cdecl> explain char *(*fptab[])()
  declare fptab as array of pointer to function returning pointer to char

> +  array_index_prompt = xstrprintf (_("Enter the index you want to explore "
> +                                     "in '%s': "),

  Enter the index of the element you want to explore in '%s'

(The user wants to explore an element of an array, not an index.)

> +  resp = gdb_readline (array_index_prompt);
> +  make_cleanup (xfree, resp);
> +  strip_leading_spaces (&resp);
> +  if (resp && resp[0] != '\0' && !is_all_spaces(resp))
> +    {
> +      struct value *elem_val;
> +      char *new_exp;
> +
> +      const char *endptr;
> +      LONGEST index = strtoulst (resp, &endptr, 10);

If you are going to call strtoulst, do you really need to strip
leading spaces?

> +  if (is_child)
> +    {
> +      new_path = xstrprintf (_("%s"), path);
> +    }
> +  else
> +    {
> +      new_path = xstrprintf (_("%s"), type_name);
> +    }

I don't think it makes sense making these translatable.

Also, a single statement in an if/else branch doesn't need braces.

> +      /* This helps in exploring types like int*, int** and the like.  */
> +      char *exp = xstrprintf (_("(%s *) 0"), str);

Likewise: types are not translatable.

> +  if (!arg_str)
> +    {
> +      printf_filtered (_("'explore' command requires an argument.\n"));

This is better:

  'explore' requires an argument

> +  choice, if any) to return to the enclosing type or value. Entering\n\
                                                             ^^
Two spaces here, please.

> +  an invalid input will also result in similar behaviour."),

"Similar" or identical?

I would actually remove the last sentence altogether.  It doesn't add
any significant information (knowing what the command does in response
to invalid input is not important), but makes the doc string less
clear than it could have been.

> +  ** "explore" and its sub commands "explore value" and "explore type"
> +     can be used to recurrsively explore values and types of
                       ^^^^^^^^^^^^
A typo.

> +     expressions. The command "explore type" can be used to explore
                   ^^
Two spaces.

The documentation part is OK with the above changes.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]