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

[Pedro Alves <palves@redhat.com>]

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