Re: [PATCH 13/24] Make "print" and "compile print" support -OPT options

Pedro Alves
2019-05-25T10:10:24Z

On 5/24/19 8:48 PM, Sergio Durigan Junior wrote:
> On Wednesday, May 22 2019, Pedro Alves wrote:
>
>> This patch adds support for "print -option optval --", etc.
>> Likewise for "compile print".
>>
>> We'll get:
>>
>> ~~~~~~
>> (gdb) help print
>> Print value of expression EXP.
>> Usage: print [[OPTION]... --] [/FMT] [EXP]
>>
>> Options:
>> -address [on|off]
>> Set printing of addresses.
>>
>> -array [on|off]
>> Set pretty formatting of arrays.
>>
>> -array-indexes [on|off]
>> Set printing of array indexes.
>>
>> -elements NUMBER|unlimited
>> Set limit on string chars or array elements to print.
>> "unlimited" causes there to be no limit.
>>
>> -max-depth NUMBER|unlimited
>> Set maximum print depth for nested structures, unions and arrays.
>> When structures, unions, or arrays are nested beyond this depth then they
>> will be replaced with either '{...}' or '(...)' depending on the language.
>> Use "unlimited" to print the complete structure.
>>
>> -null-stop [on|off]
>> Set printing of char arrays to stop at first null char.
>>
>> -object [on|off]
>> Set printing of C++ virtual function tables.
>>
>> -pretty [on|off]
>> Set pretty formatting of structures.
>>
>> -repeats NUMBER|unlimited
>> Set threshold for repeated print elements.
>> "unlimited" causes all elements to be individually printed.
>>
>> -static-members [on|off]
>> Set printing of C++ static members.
>>
>> -symbol [on|off]
>> Set printing of symbol names when printing pointers.
>>
>> -union [on|off]
>> Set printing of unions interior to structures.
>>
>> -vtbl [on|off]
>> Set printing of C++ virtual function tables.
>>
>> Note: because this command accepts arbitrary expressions, if you
>> specify any command option, you must use a double dash ("--")
>> to mark the end of option processing. E.g.: "print -o -- myobj".
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> I want to highlight the comment above about "--".
>
> Thanks for the patch. I read it, and it seems like a very good
> improvement. I just have a small point to make. Since:
>
> 1) There was a visible change of behaviour (i.e., we will now require
> the user to specify "--" when she wants to end the option processing).

Right. Though I wouldn't call that a change of behavior, because before
there was no options support at all.

>
> 2) The comment above explains this change very well.
>
> 3) It has been replicated over all of the commands that have this
> requirement,

Right. It's only "print" and "compile print" so far, though.

>
> then why don't you make the build_help routine always print it if we're
> dealing with gdb::option::PROCESS_OPTIONS_REQUIRE_DELIMITER command
> types? I think it is a nice addition to the help text.

I had thought of that but didn't do it because I thought that each command
would want to spell it out a little bit differently. E.g., for "print"
and "compile print" the difference is in the example:

Note: because this command accepts arbitrary expressions, if you
specify any command option, you must use a double dash ("--")
to mark the end of option processing. E.g.: "print -o -- myobj".

Note: because this command accepts arbitrary expressions, if you
specify any command option, you must use a double dash ("--")
to mark the end of option processing. E.g.: "compile print -o -- myobj".

... and I don't know whether "arbitrary expressions" will be the
reason for using PROCESS_OPTIONS_REQUIRE_DELIMITER with other
commands.

Since there's only two cases, I didn't think it'd be worth it to
try to come up with an abstraction until we have more uses?

BTW, one command that currently accepts "-" options _and_ works with
arbitrary expressions is the watch command:

(gdb) help watch
Set a watchpoint for an expression.
Usage: watch [-l|-location] EXPRESSION

Note how it's impossible to watch an expression named "-l".
We could solve that by supporting "--" in the watch command, so
you'd type:

(gdb) watch -- -l

So it could be argued that "watch" should be a PROCESS_OPTIONS_REQUIRE_DELIMITER
just like "print", since it has the exact same issues. It happens to only
support one option today, and probably watching "-FOO" is rare, so nobody ever
noticed this...

I _think_ that the risk of not requiring "--" for "print" is higher
than for other commands. But that's my opinion, maybe others would be OK
with requiring users to type:

(gdb) print -- -r

where they typed today:

(gdb) print -r

I'm stressing this issue because we're likely going to be stuck with whatever
we decide, practically forever. :-)

Thanks,
Pedro Alves