Feedback about users/palves/cli-options

Pedro Alves palves@redhat.com
Fri May 17 14:33:00 GMT 2019 

On 5/17/19 1:50 PM, Pedro Alves wrote:
> On 5/16/19 8:09 PM, Philippe Waroquiers wrote:
>> Hello,
>> I have retested with the last version
>> (commit e33bd5169a20460e44fab690bbba8545cc72f2e8 (HEAD -> users/palves/cli-options,
>> origin/users/palves/cli-options)
>> Find below the updated list of problems and/or suggestions.
>>
> 
> Thanks!
> 
> Most of these are fixed in my local branch.  I should push an update quite soon.

Done, I pushed what I have right now.

Some of the fixes are squashed into their right patches, but
I kept several fixes separate so you could easily see what changed.
I'll probably squash things before the next round.

I'll be working on the remaining issues you identified below.

Thanks,
Pedro Alves

> 
> More below.
> 
>> * print /x does not combine with the new options e.g.
>>     (gdb) print /x 4
>>     $2 = 0x4
>>     (gdb) print -pretty -- 4
>>     $3 = 4
>>     (gdb) print -pretty -- /x 4
>>     A syntax error in expression, near `/x 4'.
>>     (gdb) print /x -pretty -- 4
>>     No symbol "pretty" in current context.
>>     (gdb) 
>>   The problem is because the function print_command_parse_format
>>   is called with *expp pointing at the space character following --.
>>   Wondering where this is better fixed.
>>   Maybe at the lowest level (i.e. check_for_argument) : if an argument
>>   is followed by spaces, I guess these spaces should be 'eaten' by
>>   check_for_argument.
> 
> Indeed.  I wrote a preparatory patch to make that change to
> check_for_argument.
> 
> I made this work:
> 
>      (gdb) print -pretty -- /x 4
> 
> But not this:
> 
>      (gdb) print /x -pretty -- 4
> 
> Not entirely convinced of the latter, and I'd rather spend time on
> fixing the other issues for now.
> 
>> * print usage should probably better mention /FMT e.g.
>>    Usage: print [[OPTION]... --] [/FMT] [EXP]
>>   instead of
>>    Usage: print [[OPTION]... --] [EXP]
> 
> Fixed.
> 
>>
>> * compile usage does not mention the new print options:
>>    Usage: compile print[/FMT] [EXPR]
>>
> 
> Fixed.  Also made the options actually work now.  :-)
> 
>> * Some 'enum' values (on, off, unlimited, ...) are not accepted as
>>   abbreviations, while some others are.
>>   E.g.  bt -e b works
>>   while  p -e u -- 4 does not
>>    (but u will complete to unlimited)
>>
> 
> Eh, good catch.  It turns out that this inconsistency also
> exists in the corresponding "set" commands.  Unsurprising if
> you consider that parts of the code are shared between settings
> and options.
> 
> I now write a test testcase to exercise all kinds of settings, 
> var_uinteger, var_boolean, etc, etc.  For that, I added a bunch
> of new settings-framework representative "maintenance test-settings set/show"
> commands, like:
> 
> (gdb) maint test-settings set [TAB]
> auto-boolean         enum                 integer              string               uinteger             zuinteger            
> boolean              filename             optional-filename    string-noescape      zinteger             zuinteger-unlimited  
> 
> and then the testcase focus on settings parsing and completion.  
> That exercises the issue you discovered, and others!
> 
>> * bt fu<TAB> has changed of behaviour.
>>   It now gives a lot more completion possibilities.
>>   This comment can probably just be ignored, as HEAD does in any case
>>   produce not very relevant completions.
> 
> The issue here is that in master, "bt"'s completer is the default
> completer, which is symbol_completer.  Since "backtrace"'s -COUNT
> argument is handled as an expression, I had made bt's completer
> call expression_completer instead.
> 
>>   Alternatively, it might maybe be also be possible to complete on the
>>   'old option qualifier style'.
> 
> Agreed.  That's how I fixed this.
> 
>>
>> * bt accepts now 2 'counts' to  limit on the nr of frame to show.
>>   This is a little bit confusing, so maybe the interactions between
>>   the 2 might be explained in the help.
>>   E.g. explain that the below will only give 2 frames:
>>     bt -limit 2 3
>>
> 
> I haven't addressed this one yet -- I'm thinking that maybe we should
> just drop that option?
> 
>> * Now that we have nice command options completions, I wonder
>>   if the -q -c -s options should/could not be reworded as
>>     -quiet -continue -silent
>>   That will be backward compatible, and is better ergonomy.
>>   I can do that as a follow-up patch if that will help you.
> 
> Agreed.  I'll leave it as they are currently, and I'll be happy
> if you send a follow-up patch.
> 
>>
>> * display and output commands have no OPTIONS yet. 
>>   For display, I guess this means the 'struct display' has to store
>>   the option values (or maybe the string options ?).
> 
> Yeah, that seems intuitive.
> 
>>
>> * (gdb) thread apply 1 3 -ascenTAB will complete
>>   but the resulting command fails:
>>    (gdb) thread apply 1 3 -ascending p $pc
>>    Invalid thread ID: -ascending p $pc
>>   (gdb) 
> 
> Haven't looked at this one yet.
> 
>>
>> * help thread apply: the second line for the help for -ascending
>>   should probably better be indented to the right, like the first line.
>>
>> * help thread apply says:
>>    -q
>>       Disables printing the frame location information.
>>   It should be
>>       Disables printing the thread information.
>>
> 
> I've fixed this one.  The issue is that "-ascending" can only be used
> with "thread apply all".  The fix is to remove "-ascending" from
> "thread apply TID"'s completer's supported options.
> 
> Thanks,
> Pedro Alves
>

More information about the Gdb-patches mailing list